Removed unneeded code

[david-mark]

No need to reference the global object (and certainly not the way it was being done).

[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 [email protected]. 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 [email protected] if you have any questions.

[camspiers]

Might be worth reading the original commit (if you haven't already), as it does look like it was intentional:

92c84a6

[david-mark]

As a side note, can't believe the related code is now in a string literal. Assume it is eval-ed at some point. (!) Don't have the time to go over all of the changes from four days ago, but best of luck with it.

[david-mark]

Might be worth reading the original commit (if you haven't already), as it does look like it was intentional:

Looks like what was intentional?

I see several changes in the commit four days prior and also a comment that they are insane and should be removed, but those aren't directly related to this pull request; just created a conflict with a previous attempt to remove the unneeded code:

#8341

Looking at the commit cited, which is apparently when logic with the unneeded "g" (for global object) variable was added, it will work just fine without trying to reference the global object. Certainly never needed to do it with window, self, global, etc. That was the original point.

Good luck!

[david-mark]

Should definitely back out the change that puts this code in a string, but set that aside for a moment and consider what scenarios would break this code:

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

1. If whatever name replaces ${this.data.standalone} is in the function scope.
2. React is in the function scope.
3. React is not on the scope chain.

If #1 is true:

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

If #2 is true:

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

If both are true:

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

Finally, if #3 is true then the original code isn't going to do anything but pass - undefined - to f. Have made the previous assumption that nothing useful would happen in that case and therefore React must be declared somewhere prior to this code executing.

I don't have time to go over all of the AMD/CommonJS/UMD/browserify/wrapperify shenanigans (code comments admit it is insanity), but the above should shed some light (and hopefully prevent this rabbit hole from getting any deeper).

Note that no host objects are mentioned and neither is "global" as host objects are not part of the language and it's been determined that this code never runs in NodeJS (makes sense for a script called "browserify").

Also submit that there's no reason to worry about what will happen if a user edits this "browserify" file (e.g. ill-advisedly adds 'use strict' to the function). Will never happen and what if it did? What if they deleted the function altogether? Or the whole script? Trying to support such warranty-voiding actions is a very slippery slope and figure the code comments alone are enough to ward off such adventures. :)

HTH

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[david-mark]

This may be helpful as well:

https://gist.github.com/david-mark/7cf93afccf3696a4f6754b36cd6ab34e

[sebmarkbage]

We pull in from upstream if we need to but we're removing browserify anyway and replacing it with Rollup or others.

[david-mark]

I think removing that "browserify" script is definitely a step in the right direction. Will wait and see what Rollup and others looks like.