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

object.assign property enumeration order tests fail in node 0.8 with core-js #122

Closed
ljharb opened this issue Oct 21, 2015 · 9 comments
Closed
Labels

Comments

@ljharb
Copy link

ljharb commented Oct 21, 2015

The tests added in ljharb/object.assign#17 - which simply require core-js, assert that the module's hasSymbols check is true (it is), and then runs the implementation tests as a sanity check.

See https://travis-ci.org/ljharb/object.assign/jobs/86559422 - if you npm run test:shams:corejs in node v0.8 on ljharb/object.assign@b9567a3, you'll see that 'abcdefghijklmnopqrst' ends up being 'bacdefghijklmnopqrst' - however, this does not occur without core-js.

I've fixed this in ljharb/object.assign@7087754 by changing my test from using Array#reduce to the for-each module - but that implies that require('core-js/modules/es6.symbol')' is somehow causing the global Array#reduce to be modified in node v0.8 in a way that breaks property enumeration order.

ljharb added a commit to ljharb/object.assign that referenced this issue Oct 21, 2015
@ljharb
Copy link
Author

ljharb commented Oct 21, 2015

Update: it now passes locally, but is still failing in travis

@zloirock
Copy link
Owner

Interesting. I have no ideas, how core-js es6.symbol module can change this behavior. From string keys enumerators, core-js in this module overrides only Object.getOwnPropertyNamesfor filter symbols and don't changes the order, I don't think object.assign use it. core-js (currently) doesn't touches Array#reduce if it exist and, sure, not in the es6.symbol module. Looks like one more V8 bug with enumeration related additional non-enumerable properties in Object.prototype. I will explore it in the evening.

@zloirock
Copy link
Owner

Tried it, on local machine (Windows 10, Node 0.8.28) all tests passed. Without test case also can't reproduce it. As I wrote, possible one more V8 bug with enumeration order related additional prototype properties, I don't see another possible reason.

@ljharb
Copy link
Author

ljharb commented Oct 21, 2015

Yeah I'm confused also, since it passes for me locally as well, and with or without core-js, it passes everywhere else. I'd love to see it figured out, but obv you're free to close this if you can't reproduce - since a) it's only on a super ancient node, b) works everywhere without core-js, c) works everywhere so far except travis with core-js.

@zloirock
Copy link
Owner

Ok, I close it. It's old V8, not core-js, bug. The main problem with enumeration property order - native V8 Object.assign not deterministic. But looks like here result not changes at rerun. We can't guarantee completely correct property order in all engines (for example, keys, added defineProperty in FF). Anyway, thanks for this report.

@ljharb
Copy link
Author

ljharb commented Oct 22, 2015

Good point - it seems deterministic even if it's not by spec. Closing is fine.

If we ever figure out why the specific combination of node 0.8 and core-js doesn't follow spec ordering, then perhaps we can fix it :-)

@ljharb
Copy link
Author

ljharb commented Nov 10, 2015

whatever you did between 1.2.5 and 1.2.6 fixed the issue on node 0.8 - thanks!

@zloirock
Copy link
Owner

@ljharb strange... IIRC I didn't touch something related.

@ljharb
Copy link
Author

ljharb commented Nov 10, 2015

actually nvm - it's always worked locally, it's still broken in travis. carry on!

@zloirock zloirock added the v8 label Nov 10, 2015
@zloirock zloirock mentioned this issue Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants