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

ImmutableObject should tolerate arrays at deeper levels and prevent mutation on them — Fails in Safari #632

Closed
subtleGradient opened this issue Dec 4, 2013 · 3 comments

Comments

@subtleGradient
Copy link
Contributor

>> not ok 382 - utils/__tests__/ImmutableObject-test ImmutableObject should tolerate arrays at deeper levels and prevent mutation on them:DEV.

https://travis-ci.org/facebook/react/jobs/14942607#L3829

http://saucelabs.com/tests/e516f61322464a97aea25b226103ce0a

@plievone
Copy link
Contributor

plievone commented Jan 7, 2014

Offending test case is here: https://github.com/facebook/react/blob/4f57515/src/utils/__tests__/ImmutableObject-test.js#L233-L252
With webdriver-jasmine:saucelabs_ios5 it fails on both DEV and PROD.
That file has quite many if (window.callPhantom) { return; } lines, which always return success locally, instead of skipping them, so it may have invalid test cases?
ImmutableObject uses Object.freeze in DEV https://github.com/facebook/react/blob/4f57515/src/utils/ImmutableObject.js#L82 without checking availability (http://kangax.github.io/es5-compat-table/#Object.freeze), and es5-sham cannot shim it.

@jordwalke
Copy link
Contributor

Thanks for catching this. We need to detect if Object.freeze is supported. If not, then we shouldn't freeze. Also, the action items to support Arrays are:

  1. When "merging" (shallow or deep), simply move the entire array over, do not for-in them and do not recurse. Pete had ideas to be able to specify how arrays should be intelligently merged (appended, prepended, index-by-index replace).
  2. Probably something else as well.

@zpao
Copy link
Member

zpao commented Sep 23, 2014

We should test for Object.freeze, but we should probably actually just remove this from the repo, we don't use it. Also tests could be updated to use Object.isFrozen instead of depending on throwing (which only happens in strict mode, so IE9 which supports freezing but not strict mode fails).

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

No branches or pull requests

4 participants