Breaking v8 change in v0.12.3

[ljharb]

https://twitter.com/ljharb/status/598917863433342976

Array.prototype.values no longer exists in v0.12.3; it exists in v0.12.2. This will break any code relying on it.

[misterdjules]

Thanks @ljharb for the heads up, I failed to catch that during code review for #18206, my apologies for the problems caused. I'm working on a fix that I'll post asap.

[cjihrig]

@misterdjules I think we just need to restore this line.

[misterdjules]

@ljharb In https://twitter.com/ljharb/status/598936102674702336 you mentioned "I think you'd have to shim the JS or downgrade v8". Could you please elaborate? Do you see any problem with reverting https://codereview.chromium.org/647703003 instead?

[misterdjules]

@cjihrig Same as above, what do you think about reverting https://codereview.chromium.org/647703003 entirely?

[cjihrig]

@misterdjules My only concern is that Array.prototype.values() was removed from a few engines for a good reason, and that Node would be susceptible to the same issue (granted, the issue is related to Outlook, so we'd probably be fine). However, reverting seems like it would be most in line with Node's philosophy on changes between versions. It's a tough call, but I think I'd say revert.

Can we get opinions from other members of @joyent/node-coreteam?

[ljharb]

It was removed because it breaks web compat. node has no such concern. In addition, you could shim it easily by doing Object.defineProperty(Array.prototype, 'values', { enumerable: false, value: Array.prototype[Symbol.iterator] }) if there's any node hooks to do that.

The function is in the spec, so v8 will have to add it back eventually.

As for reverting that specific commit, that should work fine! Wouldn't that revert it for all consumers of v8, or would it be a node-specific fork of v8?

[misterdjules]

@ljharb We would revert that commit only in deps/v8, so only for node, binary add-ons authors, and other consumers of V8 from node.

[cjihrig]

Right, the specific problem was related to Outlook. There could be a Node user that is doing something similar. But since we already launched 0.12 with that feature, I think we should revert that commit.

[ljharb]

+1, I think that "web compatibility" reverts in v8 should always be reversed for node, since it doesn't apply :-)

[misterdjules]

@cjihrig That's my current position too.

If you have some time to put together a PR that reverts that change, that would be great, otherwise I'll get that done before tomorrow morning. I would think that the best way to revert that would be to completely revert https://codereview.chromium.org/647703003.

You might want to consult https://github.com/joyent/node/wiki/V8-upgrade-process#how-to-land-a-patch-from-upstream-v8 to do that.

[cjihrig]

@misterdjules I'd like to work on this one.

[misterdjules]

@cjihrig Please go ahead :)

[misterdjules]

Fixed by 6157697 and e23450f. Thank you!

[ljharb]

Yay, thanks! Looking forward to the release :-)

[misterdjules]

@ljharb There's a new candidate release for v0.12.4 at http://nodejs.org/dist/v0.12.4/. Could you please give it a try and let us know if that fixes the problem? Thank you!

[ljharb]

Yep, confirmed, it works!

As I said in IRC tho, the instant it appears on nodejs.org/dist, it's 100% fully publicly released, so heads up.