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.

Sign up for GitHub

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

Jump to bottom

window.__DEV__ should not be compiled to window.true #496

Closed
subtleGradient opened this issue Nov 9, 2013 · 5 comments · Fixed by #526 or #535
Closed

window.__DEV__ should not be compiled to window.true #496

subtleGradient opened this issue Nov 9, 2013 · 5 comments · Fixed by #526 or #535
Assignees
@benjamn
Labels
Type: Bug

Comments

@subtleGradient
Copy link
Contributor

This throws a syntax error in IE8.

build/modules/utils/__tests__/ImmutableObject-test.js:62

  var testProd = function(message, testFunc) {
    // Temporarily enter production mode
    window.true = false;
    it(message, testFunc);
    window.true = true;
  };

I'm not sure which code is doing this transformation, but this is clearly not what should be happening.
@sophiebits
Copy link
Collaborator

The code is here:

https://github.com/facebook/react/blob/master/vendor/constants.js#L31-L35

@ghost ghost assigned benjamn Nov 11, 2013
@subtleGradient subtleGradient mentioned this issue Nov 11, 2013
Closed
@subtleGradient
Copy link
Contributor Author

This benjamn/populist#8 made it a lot easier to find this problem.

@benjamn benjamn mentioned this issue Nov 13, 2013
Merged
benjamn added a commit to benjamn/react that referenced this issue Nov 13, 2013
@benjamn
Rewrite vendor/constants.js to use require("ast-types").traverse.
2b763fc 
Most notably, this new style of transformation gives us access to
this.parent.node, which allows us to avoid replacing identifiers that are
not actually free variables, such as member expression properties.

Closes facebook#496.
@subtleGradient
Copy link
Contributor Author

Still happening

@benjamn
Copy link
Contributor

benjamn commented Nov 13, 2013

Try doing rm -rf .module-cache/ in node_modules/react/?

@subtleGradient
Copy link
Contributor Author

That worked.
We should add that to the grunt clean task.

@benjamn benjamn mentioned this issue Nov 13, 2013
Merged
benjamn added a commit to benjamn/react that referenced this issue Dec 4, 2013
@benjamn
Establish a convention for forcing jsx rebuilds.
3527d9d 
Pull request facebook#526 updated the behavior of vendor/constants.js without
changing any source files or the bin/jsx-internal script, so files that
should have been rebuilt (like utils/__tests__/ImmutableObject-test.js)
were not automatically rebuilt (unless you knew to do `grunt clean` or
`rm -rf .module-cache` manually).

This commit allows us to bump a version number when we know the transform
toolchain has been altered in a way that will not be visible to
commoner/jsx.

With this convention, if we reset to an older revision (e.g. during a git
bisect) and the appropriate cached module files are still in the
.module-cache/, they can be used without rebuilding. That's why I prefer
this approach to just deleting the .module-cache/.

Closes facebook#104.
Closes facebook#496.
Closes facebook#530.
benjamn added a commit that referenced this issue Dec 13, 2013
@benjamn @zpao
Rewrite vendor/constants.js to use require("ast-types").traverse.
1d9a0ce 
Most notably, this new style of transformation gives us access to
this.parent.node, which allows us to avoid replacing identifiers that are
not actually free variables, such as member expression properties.

Closes #496.

Conflicts:
	package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@benjamn benjamn

Labels
Type: Bug
Projects
None yet
Milestone
No milestone
3 participants
@subtleGradient @benjamn @sophiebits