-
Notifications
You must be signed in to change notification settings - Fork 47
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
Check window first in setting global object #48
base: master
Are you sure you want to change the base?
Conversation
@jmm, ignore what I said in browserify/browserify#1282. I'm ok with this order. if you're cool with it too, then I'll merge it |
This is certainly better for the browser, but I think it can easily trigger a false positive in another environment, like if there's window && window.document && window.document.implementation ? window |
@jmm that test is too specific, and too big - remember, this gets added to every module that references The umd wrapper checks for |
I lack the confidence to say that the possibility for there to be
What makes it too specific (disregarding length)? It could be less specific than that yet still more specific than just
That seems like a significant difference. But a moot point if the premise is that |
I don't feel it's correct to look for anything besides a global variable called I take back my take-back. The correct thing to do is to leave the order as it is because NW.js creating a fake global is deliberate. I just tried @adam-lynch's browserify-nw-global-problem with a |
My interpretation is that it's all just duck typing the environment. The current test, in whatever order ( This isn't related to the DOM except in the most tangential sense. The name
Not when there's a random global This is only relevant if the first test is for something related to
The current mechaism breaks too easily considering the way web pages are composed in the real world. To me it doesn't make sense to say that it's ok for a JS application that passes a test suite, is deployed, and humming along to suddenly just break because a random attribute value unrelated to the application gets added to the document. Related: |
I have to 👍 @jmm. Testing for things that have to be on the global variable in any javascript environment is a great way to identify a global object. It seems very unlikely someone will start inserting
I originally submitted the ticket because code outside my control inserted
It says "Browserify lets you require('modules') in the browser by bundling up all of your dependencies." on http://browserify.org/. Why is node the default target environment? |
I'm having trouble imagining a real use case for insert-module-globals where |
In a browser, How does that sound? Switch the default and expose a flag? |
In other words, do a simple thing that's likely to work without testing or being overly robust and provide an escape valve for special integrations. |
This is used by Browserify with the purpose of making bundles that will run (and utilize the correct global) in any of the "target" environments (e.g. browser, node, web worker).
I'm not sure what you mean by "inside some wrapper". This will make a bundle use the wrong global in node (the assignment could be anywhere -- not necessarily "next to" the window = {};
require('./bundle');
Do you think anyone would really do that with a Browserify bundle? (I don't.) I don't think an opt-in flag is a good way to keep it from being so brittle and maybe working one execution and failing the next because some new script or module or HTML element set a random variable. But of course if there's a better solution than what's already been discussed here, that'd be great; I don't mean to discourage creativity. 😃 |
Right, sorry. I wasn't thinking clearly. Would |
@tilgovi No worries. But yeah, it's really easy for it to happen, and in code outside your control even.
It would cut down the risk of false positive a great deal in my opinion. I suggested pretty much the same thing too in an earlier comment:
|
What's the status? We just got bitten by this bug. We use Browserify to build a bundle that's supposed to be attached as a third party script to client's websites (just like Google Analytics). Turns out somebody has a variable named global on their website, so anything using global inside our bundle fails, as it expects a reference to window instead of some custom object. |
I'm still in favor of Anyone find that exceptionally scary? @parshap @substack anyone have feelings? |
I'm wondering if there's a better way to get the global object without testing Also wondering if we can use global and solve this non-trivial problem there. |
I've come across this problem, using Node Webkit, and I read all the comments from the issue and this PR. It seems to me that NW is an interesting case to study. The Webkit context of NW is an environment where
So I'd say Now, what should browserify do? It has to select The argument I've seen against this in this thread is "there's a presumption that browserify is packaging node code that should still work in node first". I'm not sure I agree. Browserify is a way to transform Node code so that it runs in the browser. The main target platform is the browser. If you want to run that code in Node, you use the unbrowserified version. I know people are also using browserify to package standalone Node packages, but I'd say that's not the main use case (it wasn't supported before 5.0.3). However, that's for browserify authors to decide. The remaining problem is the case when To sum up, IMHO:
So, what could be done to resume progress on the matter? :) Workaround for NWAs a side-node, the following workaround can help, in the case of NW: |
What is the status on this? |
This is still a problem for me... Any progress? |
I'd also propose:
which may be more robust in terms of the case where the |
I'm experiencing this problem trying to use Mocha with Karma in NW.js. I like @Zarel's solution above (#48 (comment)) as it is targetted specifically to the breaking case and is unlikely to break other things. Is there any way I can help to get something merged? |
@jmm's proposal (#48 (comment)) also seems pretty appropriate to me. |
Regarding some of @hgwood's comments:
I do not believe this is the case since NW.js 0.13: http://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#access-nodejs-and-nwjs-api-in-browser-context
This is false for me. Again, it may be an NW.js 0.13+ thing. I believe Note, of course, that when you |
Sorry for so many comments. I've done some further testing. In NW.js < 0.13 (I think; only tested 0.12.3):
In NW.js ≥ 0.13 (I think; only tested 0.22.1):
Therefore:
Purely from an NW.js perspective, then, the best order would be:
However, it would be wise to avoid situations where a variable called
This avoids any need to wrap a primitive if |
@insightfuls I was indeed referring to NW.js 0.12. Thanks for clarifying. |
This is a proposal to satisfy browserify/browserify#1189