The includes module appears to only support strings. It could leverage the fromIndex of String#indexOf instead of using slice.

keys prop fix should skip non-enum constructor properties on prototypes.

The built-in Array#indexOf supports a fromIndex, so does Underscore. It would be rad if amp did too. It can be useful for creating other methods like one that pulls a value from an array.

suggested by @legastero

https://www.npmjs.org/package/lodash-node

Various implementations of trim consider different whitespace as trimmable because it's based on the unicode standard which shifts regardless of the lang spec. It would be better to simply use the fallback and avoid inconsistencies.

_{I'm including these three distinct issues in a single ticket as I believe that they should be resolved consistently.}

addClass, hasClass, and removeClass will have inconsistent behavior between browsers when called with a class name such as 'foo bar'.

addClass

addClass(el, 'foo bar')

if the classList API is supported, a DOMException will be thrown
if the classList API is not supported 'foo bar' will be added in a way that can cause class duplication.

hasClass

hasClass(el, 'foo bar')

if the element happens to have those classes in that order, it will return true.

removeClass

removeClass(el, 'foo bar')

if the classList API is supported, a DOMException will be thrown
if the classList API is not supported, 'foo bar' will be removed only if those classes appear in that order.

Recommendation

The API doesn't specify what should happen for trying to add multiple classes in a string. I believe that this should be fixed in a way to support utility to authors, even though it will cause a slight loss in performance and increase in code size.

I believe that it is obvious that addClass(el, 'foo bar') should be equivalent to addClass(el, 'foo', 'bar'), and that removeClass(el, 'foo bar') is equivalent to removeClass(el, 'foo', 'bar').

I would also argue that hasClass(el, 'foo bar') should be treated as equivalent to hasClass(el, 'foo') && hasClass(el, 'bar'), however I am understanding of arguments for treating it as hasClass(el, 'foo') || hasClass(el, 'bar'), or explicitly disallowing that behavior to avoid possible author confusion.

That said, I am in favor of hasClass(el, 'foo bar') and the introduction of hasClass(el, 'foo', 'bar'), and hasClass(el, ['foo', 'bar']) as a means of specifying that an element has all of the listed classes.

as mentioned in AmpersandJS/ampersand-state#131

• add-class
• has-class
• remove-class
• bind
• debounce
• delay
• limit-calls
• once
• run-after
• contains
• each
• every
• filter
• map
• size
• some
• difference
• flatten
• index-of
• last
• range
• unique
• clone
• defaults
• extend
• has
• invert
• keys
• matches
• pairs
• property
• values
• escape
• includes
• to-camel-case
• trim
• is-arguments
• is-array
• is-boolean
• is-date
• is-empty
• is-function
• is-nan
• is-null
• is-number
• is-object
• is-object-equal
• is-regexp
• is-string
• is-undefined
• iteratee
• result
• unique-id
• create-callback
• internal-flatten

The is-boolean module doesn't match objects with the [[Class]] of Boolean while others like is-object-equal and is-number match [[Class]] values.

I get "Error: Cannot find module 'uglifyjs'" when I run npm install. I am a new00be at npm, but it looks as if the module may be unpublished https://registry.npmjs.org/uglifyjs

Thoughts

is-empty should use amp-keys are it's missing out on the object fixes of keys.

capitalize can stand on its own outside of to-camel-case.

unique has isSorted optimization but it's missing from index-of. Maybe remove it from unique?

a la https://github.com/AmpersandJS/ampersand-router/blob/master/NOTICE.md

@HenrikJoreteg the tests pass, because there are no tests :(

The name is a bit odd as it works with primitive values.

When I ran npm run folders on an item I added to modules.json the readme.md file was not created, so the instructions to 'just change the attribution' didn't make sense.

Also, running readmes clobbers all the readmes that happen to have the attribution changed in it.

Usually doing less by default and opting into more expensive operations is the groovy way to go. This would also align with Array#concat which is commonly used as a shallow flatten.

Class name values may contain other whitespace than \x20.
It can contains tabs and other whitespace ([\t\r\n\f]).

Similar to has, the property should allow obj to be nullish.

Is there a way to find the first match in a collection/ array without iterating through the entire collection? I can currently use filter and then get the first element in the result array. However, this is not very fast for large collections.

The indexOf module forks for the built-in Array#indexOf but this creates an inconsistency with those without because native skips holes while the fallback does not. So indexOf(Array(1), undefined) will return a different result in various environments.

Suggested by @nlf.

i would feel better if you grouped things in loglcal combinations
the type comparisons in one module
the object clone/merge in another

but i think having a separate module for every method feels like taking it too far

take is-nan:

var isNumber = require('../is-number');

module.exports = function isNaN(obj) {
    return isNumber(obj) && obj !== +obj;
};

When it's in this repo, it can require ../is-number, because that's the relative path to is-number... But if I do npm install is-nan in my own project, how will it work?

result should execute defaultValue if it's a function.

extract unescape into its own module.

amp-find would work exactly like amp-filter, except it would only return the first value that passes truth test, undefined if nothing passes.

amp-find-where would be a combination of amp-find and amp-matches

amp-find(collection, function, [context])
amp-find-where(collection, attributeObject)

optionally amp-find-where could instead be called amp-find-match

It's a utility module as well. Would be a clean and consistent approach to merge it into amp instead of having it as an individual module. What do you think?

It requires a small change to the objects extend module already in amp but that could be a simple name change, for example:
amp-object-extend
amp-class-extend

the test file right now is a stub it seems, thanks to @jvduf for noticing

since partial isn't supported it would be nice if delay was closer to it's original version. This way it wouldn't require bind and could have partial application of args. If bind is needed devs could always use bind in combo with delay.

The amp-trim regexp uses \s which differs from engine to engine on what whitespace is matched. It's better to be explicit in this case or you get into similar situations as the native trim use.

limit-calls doc still as before reference.

If there's nested deps, it happens in node land. This could pave over another instances unique id counter.

defaults and extend should have for-in fixes similar to keys.

Duck-typing splice with length is what dev tools in Chrome, Firebug, WebKit do and because of that libs like jQuery and MooTools have it as well. So by supporting that in is-empty you get support for more than just jQuery too.

I was going to update some libraries using amp-merge in favor of the suggested lodash.merge. Once finally looking at the two methods, they expect different inputs, and give different outputs. You can make them work the same, but the idea of amp-merge came out of laziness and not wanting to confuse using _.extend({}, whatever) in our code. This also replicates the now depreciated helper method that was included in react around version 0.10. You can see it's use here https://github.com/facebook/flux/blob/09309a4aa4284e43b20737c3b7cd88ff9439ce89/examples/flux-todomvc/js/stores/TodoStore.js#L22

I can continue to use amp-merge as it exists today, however the documentation suggests using lodash. Is @jdalton interested in a new method with this use case?

the CI build failed as a result of Zuul tried to run tests in saucelabs, however no saucelabs credentials were provided.

as per #53

I feel like we should clarify why this is actually an issue?

amp-add-class includes amp-has-class which appears to be based off of JQuery. When using amp-add-class on an SVGElement (or any object derived from one), the method hasClass() fails.

This because of the following line.
var cName = (isString(el) ? el : el.className).replace(whitespaceRE, ' ');
Once it sees that the element is an object it tries to use className which doesn't work for SVG Elements in IE10.

A workaround is to simply use element.classList.add('classname') and element.classList.remove('classname'). Again this was only tested in an IE 10 browser in a Windows 8 environment, testing in chrome and firefox

(edited 0516PM EST) "testing in chrome and firefox" did not have this issue, sorry for that.

toCamelCase should have a guard arg so it can be used with _.map without issue.

http://code.gar.zone/?gist=bb1c443301d36d7fef5f

It returns a function instead

Should the license file end up in the actual modules too?

The docs on that are pretty light as far as what uses it, but more importantly what the expected/allowed parameters and their syntax are.