Shims breaks crypto

[codebling]

Simply including a collection breaks the crypto module.

The crypto.getHashes returns an empty array if called after a collection is loaded when it would usually return an array of supported hashes.

$ node
> var Map = require('collections/map')
undefined
> require('crypto').getHashes()
[]
>

I see #116, #95 and #94 but it looks like no shim fixes ever made it into master.

Based on how long it took to track this down, I unfortunately have to opt not to use this otherwise fantastic module. I'm not sure this will be fixed but I'm documenting this so that perhaps someone else can find it and save time debugging.

[Bazze]

I can also say that including a collection also breaks some functionality in the balderdashy/sails framework which relies on the req.params.all() function. It took me hours to debug this, it seems crazy to alter the global Array prototype like this.

Reverting to [email protected] seems to do the trick, let's hope that the library I rely on that uses collections will work with an old version :)

[marchant]

@codebling, do you have sense of how collections impacts crypto getHashes() ? I assume that is in node.js?

[kriskowal]

In v1 of collections, I mis-predicted the behavior of some methods that ES6 introduced. These shims were intended to be forward compatible, but the future did not turn out how I expected. Particularly, some methods return iterators instead of arrays. I don’t know what specifically broke, but I abandoned shimming in v2. v3 skips over the changes I made in v2 because Montage can’t needed to make a few breaking changes, but not the breaking changes as made it into v2.

[codebling]

@marchant, yes, it is the way that the shims alter the global primitives (specifically the Array prototype, I believe. After many hours of debugging, having finally identified Collections.js as the source of my woes, there was no point spending any more time debugging to see exactly how it was broken, given that shims have been fixed in another branch).

@kriskowal I saw that you made some shim replacements/fixes in v2, and I'm (now) aware that those fixes didn't make it into v3 and later versions or ever get merged to master branch. Since no issue has ever been opened that details some of the (potentially many) problems that shims (specifically, the global prototype modifications in the collections.js shims) introduce, I felt that one needed to be, if only to document the issue for others. It looks like you did a great job replacing shims (I tested your version and it does indeed fix the issue), it just didn't make sense for me to go with a fork which was possibly not in active development when I was able to get passable functionality from the ECMAScript 6 Map. Regardless, thank you for your work and contributions to this project and to open source in general !

For what it's worth, I think it would be nice to see the shimless v2 branch merged into v3, or vice-versa. Seems silly to keep what seems like a major bugfix branch separate from the main devel, but I'm sure that everyone has their reasons!

[marchant]

what we've done in v3+ are primarily performance optimization and to re-align shims of ES objects like Map and Set to latest standards, including changes impacting iterators. It's setup in a way that IE10 and IE11 use the shim rather than built-in objects, IE11 doesn't have iterators and they are impossible to shim and since it's a dying breed... that will do. We have production Apps using it from that stand point. I'll check with @kriskowal if there's a path to remove global shim for v3+

[codebling]

As far as I could tell, the shims were not selectively loaded, they were always loaded and always changing the global prototypes. I could go back and find out exactly what about the Array shim was misbehaving, such that it could be fixed, but it seems like modifying the built-in prototypes is a problematic approach. See also #145 and #139.

kriskowal seems to have eliminated the shims by factoring out the global object operators used and creating consistent internal copies (see commit log here). This seems like the correct approach to me, and will definitely work for v3+. Alternatively, a nuanced version of kris' fix would be to create consistent internal copies of the global objects before shimming them, then apply the shims to those and var Object = require('./shims/object') instead of using the built-in Object.

[vith]

Just spent all day debugging some insanity caused by these shims.

Here's a minimized test case: https://github.com/vith/collections-shim-chaos

If I remove the collections import from my tests everything works as expected. With the shims, one version of the test stalls forever; another one breaks when the test library attempts to pretty-print the error message.

Also when I try to debug the break.js test, I see an object exist in one stack frame and magically become undefined when it's passed as a function argument. So maybe it's managed to break babel. I'm not sure.

Would I be out of line to ask for a warning about the shims to be added to the project README if they're going to be kept?

[vith]

Now that I removed the shims from a codebase, flow-runtime assertions that used to spuriously succeed suddenly started working correctly too.

[kriskowal]

Totally reasonable to expect a prominent warning in the Readme, suggesting that you use collections@^2 to get the version that does not have shims. @Stuk and I are working to release `@collections` namespaced packages out of a monorepo and update collectionsjs.com to advise using those to resolve this problem.

…

On Thu, Oct 12, 2017 at 4:47 AM Jason Papakostas ***@***.***> wrote: Now that I removed the shims from a codebase, flow-runtime assertions that used to spuriously succeed suddenly started working correctly too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#162 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADrhr6ZhoVp_roZnifHNRgkLP-7y8Zeks5srdIWgaJpZM4KQnLV> .

[hthetiot]

Duplicate #185

[codebling]

@hthetiot still not fixed in 5.1.2. See also #185

$ node
> require('crypto').getHashes().length
46
>
(To exit, press ^C again or type .exit)
>
$ node
> var Map = require('collections/map')
undefined
> require('crypto').getHashes().length
0
>

[codebling]

@hthetiot should I open a new issue for this?

[codebling]

See also issues #36 #70 #94 #95 #116 #139 #145 #162 #165 #169 #178 #182 #185 #197 #215 #220 and PRs #94 #95 #116 #173 #189 #212. Branch v2 fixes these issues by avoiding global object modification.