Fix NodeJS support (broken in previous change)

[david-mark]

Before submitting a pull request, please make sure the following is done...

1. Fork the repo and create your branch from master.
2. If you've added code that should be tested, add tests!
3. If you've changed APIs, update the documentation.
4. Ensure the test suite passes (npm test).
5. Make sure your code lints (npm run lint) - we've done our best to make sure these rules match our internal linting guidelines.
6. If you haven't already, complete the CLA.

Apparently already caused at least one issue for no reason. :(

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

[gaearon]

Apparently already caused at least one issue for no reason. :(

Can you elaborate? It's not clear what this is fixing.

[david-mark]

Note that there was an issue created by the improper use of host objects when just need a reference to the global object.

Elaborated on here:

https://gist.github.com/david-mark/31d3a791af085f6fe8c177452c1d2d40

Thanks!

[david-mark]

Actually, I made one mistake in the pull request as would need to put:

var global = global || this;

...at the top (in the global context) for NodeJS support. Updated here and in the linked review.

[david-mark]

Interested to know how the initial change passed both checks. See that the new one has as well, but that was expected. Initial change must not have been checked with NodeJS.

Understand that the logic before these changes made no sense at all and caused an issue that was apparently patched. The current version may well "work", but realize it is only working by coincidence and is clearly much longer and more complex than was needed. Also subject to the whims of host objects, which is the main point.

Anyway, good luck!

[gaearon]

Our CI infrastructure doesn't currently test the UMD builds, just the code. (We don't change UMD wrappers more than once a year or so.)

[david-mark]

Wherever that code came from (UMD?), it was clearly written without much understanding of how the ECMAScript language works. I'd stop using code from that source. ;)

[sophiebits]

It's conceivable that someone would use a UMD build in a browser environment and add a use strict – perhaps an odd mistake, but one that people make (jashkenas/underscore#2152) – in which case this would be undefined and global would be missing.

Let's check global then self then this, or global then this than self, whichever you prefer. (React doesn't usually run in a worker but you could want to so let's support that.) global was recently standardized by TC39 so it will be in all new browsers soon.

[david-mark]

I'd say if they add 'use strict', that's their own problem. They would then need to add this to the top:

var global = global || this;

...as mentioned in the attached technical review. Of course, I have no idea whether your "compiler" for these scripts wraps them in additional one-off functions. Regardless, anybody that would add 'use strict' inside of that one-off either knows what they are doing or deserves whatever they get IMO.

Could use self without window as it is the same as window.self (outside of Web workers of course), which references window; but this still requires host objects where none should be needed. There's just no standard for the assumption that the window is the same as the global object.

Anyway, glad you are responding and receptive to changing it in some way. Let me know one way or the other and I'll update it. Either change would be better than none. Thanks!

[zpao]

What problem are you fixing here? I'm not saying it's not a fine thing to take but we haven't heard of any issues with this wrapper in the past, and a virtually identical wrapper is being generated by browserify in the react dist file (and also inside the code that f is calling). Generally it's recommended that you don't load the dist files in Node but they do currently work as the block you are modifying doesn't get entered.

For reference, this UMD wrapper is based on the one that browserify generates (with a tweak as mentioned in the giant comment above), which comes from here: https://github.com/ForbesLindesay/umd/blob/master/template.js.

[sophiebits]

In particular, module.exports is defined with most ways of loading this compiled file into node which side-steps this global-finding code. How are you loading it such that it's not?

[david-mark]

zpao,

It's not a matter of whether it fails somewhere at the moment (though the strategy caused a previous failure). The code is simply wrong-headed and unnecessarily long-winded. See the attached review. In short, there's no reason to resort to referencing host objects instead of referencing the global object as the language provides. That's what got you into trouble on the previous issue.

The UMD thing gets the same treatment.

ForbesLindesay/umd#35

Suggest that if they balk at that change you should stop relying on them for this bit. Apparently you don't even need NodeJS support here (see next message). Their unit tests are as confused as the code, so the pull request is failing checks. Certainly ironic. :)

[david-mark]

spicyj,

I'm not loading anything. If you are referring to the correction of my initial pull request, it was simply a mistake and I subsequently realized I had broken any environment that relied on the "global" variable (e.g. NodeJS and nothing else that I've heard of).

And if I understand zpao, then this code doesn't even run in NodeJS, so can be reduced further by just referencing the - this - object. And I wouldn't worry at all about what might happen if somebody edits the code (e.g. adds 'use strict'). That's a very slippery slope. ;)

Of course, if you can confirm that this module does not get wrapped by some other module (and thus retains its global scope), we can just put this at top and be done with it:

var React;

Furthermore, if you can just confirm that this script is worthless without the above present in some other script that must be loaded or concatenated with it, there's no need to reference the global object at all. ;)

[david-mark]

As we haven't finished the discussion, the "needs-revision" label would seem premature. Please review last couple of comments and let me know what you want to do with this.

[david-mark]

zpao,

I see the change you made, adding another reference to "g". Contend that "g" is not needed at all.

Replace:

g.${this.data.standalone} = f(g.React);

...with:

${this.data.standalone} = f(React);

...as there's no chance of this code doing anything useful if "React" is not already in scope. Can get rid of all that window, self, global, etc. code that you inherited from "UMD"; don't see them fixing their code yet as their unit tests are trained to confirm the mistake. :(

ForbesLindesay/umd#35

[david-mark]

Closing as there is new a conflict and it has been determined that this "browserify" script doesn't run in NodeJS (unsurprising given the name). Will create a new one...

[david-mark]

Follow up here:

#8428