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

Drop usage of collections, it breaks globals #160

Closed
dignifiedquire opened this issue Dec 9, 2016 · 15 comments
Closed

Drop usage of collections, it breaks globals #160

dignifiedquire opened this issue Dec 9, 2016 · 15 comments
Assignees

Comments

@dignifiedquire
Copy link

I landed here, because of a recursive dependency. The gist is that the mere requiring of the module collections randomly breaks code in unrelated modules because it modifies globals. It is documented that this breaks things like crypto from node core but there does not seem to be a fix coming: montagejs/collections#162

@dignifiedquire
Copy link
Author

The specific issue is in this line: https://github.com/kriskowal/q-io/blob/v1/fs-mock.js#L7

@zamb3zi
Copy link

zamb3zi commented Dec 14, 2016

This is a big problem for me as well. My specific issue is collections/shim-array.js assigning to Array.prototype.toObject which interferes with LoopBack.
This is a dealbreaker for using q-io in our project.
Who knows what other havoc collections will have on the rest of my app. Can we get away from it?

@kahluagenie
Copy link

kahluagenie commented Dec 15, 2016

Just ran into an issue with Array.prototype.find() being overwritten by collections/shim-array.js as well.

@kriskowal
Copy link
Owner

PR welcome that relieves the dependency on collections. q-io@v2 depends on q@v2 and collections@v2, which does not have shims, but migration will not be practical for many applications.

@nite
Copy link

nite commented May 3, 2017

So - in reference to your last comment @kriskowal: 'q-io@v2 depends on q@v2 and collections@v2, which does not have shims' - why is q-io v2 not latest on npm?

@hthetiot
Copy link
Collaborator

hthetiot commented Dec 7, 2017

See montagejs/collections#185 (comment) update

@kriskowal
Copy link
Owner

@nite It’s become evident that v2 of Q, Q-IO, and Collections are never going to be "latest". It’s my experience that making breaking changes, even with care, is not really a service to the community, so I’ve backed away from all of these endeavors. Version ^1 is ink. I continue to stand by the first line of my last comment:

PR welcome to relieves the dependency on collections in the version 1 train.

@kriskowal
Copy link
Owner

I should note that @Stuk and I are in the process of upreving collections by publishing individual collections (sans shims) in the npm @collections organization, out of the https://github.com/kriskowal/collections "monorepo".

@henhal
Copy link

henhal commented May 15, 2018

I spent quite some time not understanding how I could get -1 from an Array.find call, until I realized q-io had messed up the Array prototype by including a really old version of collections with out of date shims. For example, there is a bug in collections so that Array.find behaves like indexOf (!) on collections 0.2.0, and there is also no check for whether the prototype already contains the added methods, hence my native Array prototype was overridden, with catastrophic results.

If you cannot easily get rid of the collections behavior, can you at least bump up the version, 0.2.0 being ancient and all (5.1.2 is the latest at time of writing)?

Personally I will probably stop using q-io as of now since nothing seems to be happening to address serious issues like this for several months.

@hthetiot
Copy link
Collaborator

hthetiot commented Jul 11, 2018

We fixed couple of issue in collections v5.1.3

https://github.com/montagejs/collections/blob/master/CHANGES.md#v513
montagejs/collections#185

@hthetiot
Copy link
Collaborator

If you cannot easily get rid of the collections behavior, can you at least bump up the version, 0.2.0 being ancient and all (5.1.2 is the latest at time of writing)?

ACK

@hthetiot hthetiot self-assigned this Jul 11, 2018
@hthetiot
Copy link
Collaborator

Always funny to see a group complain and another making PR to fix the problem.
Noting also the one that talk the much are not the one that code that much.

@hthetiot
Copy link
Collaborator

Example of person that made PR to remove collections:

@hthetiot
Copy link
Collaborator

@hthetiot
Copy link
Collaborator

Merged #168 that remove collections in favor of es6-set, I will release today

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

No branches or pull requests

7 participants