Remove need for global workaround and make Browserify happy #84
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…that:
* components making use of `window` can actually do it without the trick found in the `components.js` file of the dummy test app
* components/files packaged as CommonJS modules by browserify (using browserify-rails for instance) can successfully use `global`
For this second point, this fix is necessary because of the self-calling function browserify uses to wrap code that uses the `global` variable:
```ruby
(function (global){
global.MyComponent = React.createClass({ ... });
}).call(this,typeof self !== "undefined" ? self : typeof window !== "undefined" ? window : {})
```
If `window` was used instead of `global`, it would still need the fix or the workaround found in the tests' dummy app as mentioned.
|
I'd be convinced with that manual testing you mentioned. |
| @@ -22,7 +22,7 @@ def self.render(component, args={}) | |||
|
|
|||
| def self.setup_combined_js | |||
| <<-CODE | |||
| var global = global || this; | |||
| var global = global || this, self = self || this, window = window || this; | |||
There was a problem hiding this comment.
Nit, let's do each of these on their own lines with their own var. That's more in line with our other styles (and matches both mine and FB's preferred style).
|
Thanks for all the comments @zpao (and for your help on IRC ^^), I'll correct and push again shortly. |
|
There you go @zpao :) |
zpao
added a commit
that referenced
this pull request
Sep 15, 2014
Remove need for global workaround and make Browserify happy
|
Thanks! |
|
Can I register a dissenting opinion after-the-fact? There's a lot of code out there using:
To make sure that code that can't work server-side will only run client-side. |
|
@zpao, has @jwietelmann's objection been taken into account? |
jbroadice
added a commit
to jbroadice/react-php-v8js
that referenced
this pull request
May 23, 2015
DevHunter128
added a commit
to DevHunter128/react-php-v8js
that referenced
this pull request
May 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Define
selfandwindowglobal objects in the combined JS code so that:components in Coffee files can really use
windowinstead of usingglobalto become accessible, thus removing the need for thevar self, window, global = global || window || self;trick incomponents.jsto have the code work both on server & clientscomponents/files packaged as CommonJS modules by browserify (using browserify-rails for instance) can successfully use
globalif they want toFor this second point, this fix is necessary because of the self-calling function browserify uses to wrap code that accesses the
globalvariable:If
windowwas used instead ofglobal, it would still need the fix when compiling on the server, aswindowwould not be defined otherwise.I have not included tests, as I am not sure how to test this...
Maybe what could serve for testing would just be to remove the
var self, window, global = global || window || self;line fromcomponents.jsin the dummy app, changeglobal.Todo = Todotowindow.Todo = Todoinapp/assets/javascripts/components/Todo.js.jsx.coffeeand be satisfied that tests are still successful?Open to suggestions to complete the PR :)