Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

deps: revert v8 Array.prototype.values() removal #25328

Closed
wants to merge 2 commits into from
Closed

deps: revert v8 Array.prototype.values() removal #25328

wants to merge 2 commits into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented May 15, 2015

The Node 0.12 line was initially released with a version of v8 that included Array.prototype.values(). In #18206, v8 was updated to a version that dropped support for values(). https://codereview.chromium.org/647703003 removed this method because it causes problems with some versions of Outlook Web Access. This commit reverts the removal of Array.prototype.values().

Closes #25324.

@misterdjules
Copy link

I would suggest maybe adding a regression test similar to:

/*
 * Regression test for https://github.com/joyent/node/issues/25324.
 */

var assert = require('assert');

var array = ['foo', 'bar', 'baz'];
var iterator = array.values();

var nbValues = array.length;
var nbIterated = 0;

for (var item of iterator) {
  ++nbIterated;
}

assert(nbIterated === nbValues);

so that we avoid breaking this in the future in v0.12.

Otherwise LGTM.

@ljharb I imagine you found out about #25324 by running some code from es6-shim? If so, could you please run that code against that PR and check if that fixes it for you?

Thank you!

@cjihrig
Copy link
Author

cjihrig commented May 15, 2015

@misterdjules added a test.

@ljharb
Copy link
Member

ljharb commented May 15, 2015

Yes, I clone the repo and run npm run test-native. I'm not sure how to install node easily from a non-release though.

@@ -21,9 +21,9 @@ function TestTypedArrayPrototype(constructor) {
assertFalse(constructor.prototype.propertyIsEnumerable(Symbol.iterator));

assertEquals(Array.prototype.entries, constructor.prototype.entries);
assertEquals(Array.prototype[Symbol.iterator], constructor.prototype.values);
assertEquals(Array.prototype.values, constructor.prototype.values);
Copy link
Member

Choose a reason for hiding this comment

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

These Symbol.iterator tests should still be here though.

Copy link
Author

Choose a reason for hiding this comment

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

I simply did a revert on the commit that removed Array.prototype.values(). What the v8 team does with their tests is their own business :-)

@misterdjules
Copy link

@cjihrig Thank you! Do you have some time to run the tests that @ljharb mentioned and make sure they pass with these changes?

I would also put the node test in a separate commit, but that's a minor nit, I'm ok with it like that.

@cjihrig
Copy link
Author

cjihrig commented May 15, 2015

Ran the tests. None of the remaining failures appeared to be related to Array.prototype.values().

The Node 0.12 line was initially released with a version
of v8 that included Array.prototype.values(). In
#18206, v8 was
updated to a version that dropped support for values().
https://codereview.chromium.org/647703003 removed this
method because it causes problems with some versions of
Outlook Web Access. This commit reverts the removal of
Array.prototype.values().

Original commit message:

Revert "Version 3.28.71.17 (merged r24706, r24708)"

This reverts commit 529541ecb58fd0d6df4dfbe41d01bff9ae21ff06.

Conflicts:
	src/version.cc
This commit adds a regression test for
#25324
@cjihrig
Copy link
Author

cjihrig commented May 15, 2015

@misterdjules split the Node test into a separate commit as you requested.

@DomT4
Copy link

DomT4 commented May 21, 2015

What's the status of this? Is 0.12.4 likely to land shortly after this is merged?

@ljharb
Copy link
Member

ljharb commented May 22, 2015

+1, I think this needs to go out ASAP since it was a breaking change in a patch version

@misterdjules
Copy link

@DomT4 @ljharb Yes, current plan is to ship this as v0.12.4 as soon as recent issues with Windows XP are resolved.

The relevant Windows XP issues are #25347 and #25348. The current status on these issues is that we managed to reproduce the problem, we tested that the suggested fix fixes the problem, we just want to go the extra mile and understand what caused the regression before we close these issues.

Hopefully that won't be too long.

@misterdjules
Copy link

LGTM. Thank you @cjihrig 👍

cjihrig added a commit that referenced this pull request May 22, 2015
The Node 0.12 line was initially released with a version
of v8 that included Array.prototype.values(). In
#18206, v8 was
updated to a version that dropped support for values().
https://codereview.chromium.org/647703003 removed this
method because it causes problems with some versions of
Outlook Web Access. This commit reverts the removal of
Array.prototype.values().

Original commit message:

Revert "Version 3.28.71.17 (merged r24706, r24708)"

This reverts commit 529541ecb58fd0d6df4dfbe41d01bff9ae21ff06.

Conflicts:
	src/version.cc

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: #25328
cjihrig added a commit that referenced this pull request May 22, 2015
This commit adds a regression test for
#25324

Reviewed-By: Julien Gilli <julien.gilli@joyent.com>
PR-URL: #25328
@misterdjules
Copy link

Landed in 6157697 and e23450f. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants