Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove collections dependency and es6-set instead #131

Closed
wants to merge 1 commit into from

Conversation

faridnsh
Copy link
Contributor

Since collections module modifies native objects, it's only good
to be used as a top dependency of an application and not modules that
it's using.

The problem is that this will conflict if native objects has been
modified by another module, let's say something like sugar or just
another version of collections. When you have two of these being used in
your app, you might be in trouble, where some code depends on a
behaviour of a method implemented by one module but actually the method
had been silently overriden by another module having a different
behaviour.

@faridnsh faridnsh force-pushed the remove-collections branch 2 times, most recently from c58d9b5 to e617577 Compare January 14, 2015 10:34
if (Object.has(keys, method)) {
return Object.get(methods, method)(request);
if (keys.indexOf(method) !== -1) {
return methods[method](request);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O(n) vs O(1). Let’s use Object.prototype.hasOwnProperty.call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And one less line... Fixed it :)

@kriskowal
Copy link
Owner

With version 2 of Collections, I’m moving away from shims in favor of polymorphic operators (e.g., pop-swap, pop-equals, pop-compare). Q-IO version 2 already depends on Collections, although the change is incomplete. It makes me sad for Q-IO v1 to diverge, but this does seem like a sensible change.

Since [collections module][1] modifies native objects, it's only good
to be used as a top dependency of an application and not modules that
it's using.

The problem is that this will conflict if native objects has been
modified by another module, let's say something like [sugar][2] or just
another version of collections. When you have two of these being used in
your app, you might be in trouble, where some code depends on a
behaviour of a method implemented by one module but actually the method
had been silently overriden by another module having a different
behaviour.

[1]: http://collectionsjs.com
[2]: http://sugarjs.com
@hthetiot
Copy link
Collaborator

hthetiot commented Apr 5, 2017

+1 merge pls

@kriskowal
Copy link
Owner

Adding @hthetiot as a collaborator. Send me your npm handle and I'll grant you publish as well.

@hthetiot hthetiot added this to the Future milestone May 5, 2017
@hthetiot
Copy link
Collaborator

@alFReD-NSH Can you rebase ?
simply ignore my collections update in package.json.
Also if we merge that should we name that v3 ? @kriskowal

@hthetiot
Copy link
Collaborator

hthetiot commented Jul 11, 2018

Thanks @alFReD-NSH for great work.
On v1 (master) i have couple of fixes that i merged into a new PR here

Feel free to re-open this PR with more changes if needed, in the meantime we may merge #168

@hthetiot hthetiot closed this Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants