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.

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

ReferenceError when using npm package in the browser; get global object properly #71

Closed
jedwards1211 opened this issue Jun 24, 2016 · 15 comments

Comments

@jedwards1211
Copy link
Contributor

jedwards1211 commented Jun 24, 2016

I know you might to say "use bower"...but no. React devs use npm for everything these days, including for the browser, and we're used to npm packages working in both node and the browser (i.e. via webpack).

Usually when I need access to the global object, I do

var global = (new Function("return this"))()

Works in node and the browser.

Alternative you could call your bootstrap function with

(typeof global !== 'undefined' ? global : this)

(see http://stackoverflow.com/questions/3277182/how-to-get-the-global-object-in-javascript)

Whether you want to use that or some other solution, please make sure that src/lolex-src.js works even if accessing global would cause a ReferenceError.

@jedwards1211 jedwards1211 changed the title global is undefined when using npm package in the browser global throws ReferenceError when using npm package in the browser Jun 24, 2016
@jedwards1211 jedwards1211 changed the title global throws ReferenceError when using npm package in the browser ReferenceError when using npm package in the browser; get global object properly Jun 24, 2016
@fatso83
Copy link
Contributor

fatso83 commented Jun 28, 2016

Would not dream of saying such a thing. Who uses Bower anyway these days? ;-) Just update #72 to pass the linting and this should be fixed.

@jedwards1211
Copy link
Contributor Author

Ah okay, I thought maybe not many folks at sinonjs were using Webpack, since sinon (before 2.0.0-pre) isn't Webpack-compatible. Working on fixing the jslint error...

@fatso83
Copy link
Contributor

fatso83 commented Jul 14, 2016

Fixed by #72 (47b30ed)

@fatso83 fatso83 closed this as completed Jul 14, 2016
@fatso83 fatso83 reopened this Jul 15, 2016
@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2016

I messed up when rebasing and merging in your changes, as I didn't run the linting (which failed due to migrating to ESLint where we disallowed Function calls), and I had to revert the commit.

While just disabling that eslint rule would be fine, I went on to inspect this further, seeing if the Function call was necessary or not (or if we could just use your other solution), and I devised a test script to see if thing still worked:

var lolex = require("./src/lolex-src.js");

var clock = lolex.install();
fn = function() { console.log('should show immediately'); }
setTimeout(fn, 15000); // Schedules with clock.setTimeout
clock.tick(20000);

I compiled this using webpack --output.file out.js foo.js and also created a index.html with these contents:

<script src="./out.js" ></script>

What struck me was that this works without your patch in both the browser and in Node, so I am not sure which problem you are fixing? Could you please come up with the details necessary to recreate a failing testcase in the master branch?

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 15, 2016

Sure, I wonder if it depends on the webpack config? I just checked and if I go into my chrome console and evaluate global I get a ReferenceError. So maybe webpack is polyfilling it? Or babel?

@jedwards1211
Copy link
Contributor Author

Yes, here it is: https://webpack.github.io/docs/configuration.html#node
Now to find out why that wasn't taking effect for me...

@jedwards1211
Copy link
Contributor Author

In any case wouldn't you want lolex-src.js to work standalone in the browser, without being built into a webpack bundle?

@jedwards1211
Copy link
Contributor Author

we could probably go with typeof global === 'undefined' ? this : global instead...

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Jul 15, 2016

Okay, I think what happened was I was using sinon < 2.0.0 in the browser (with Karma), and since sinon < 2.0.0 is not webpackable, I was loading it and lolex as webpack externals, hence no polyfill for global was getting injected into lolex. Make sense?

@jedwards1211
Copy link
Contributor Author

In addition I think usage of mochify in your tests is also polyfilling global in the code that gets run in phantomjs.

@jedwards1211
Copy link
Contributor Author

Okay, repro is simple: just paste lolex-src.js into a Chrome console and hit enter. You'll get Uncaught ReferenceError: global is not defined(…)

@jedwards1211
Copy link
Contributor Author

sigh the JS world can be so confusing these days. Yes, I was trying to use lolex as a webpack external so that I could use sinon 1.7 as a webpack external. I hadn't noticed that you basically require lolex to be built with browserify or webpack to use in the browser. In any case the global || this at the bottom of lolex-src.js is probably pointless

@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2016

It already does. It's in the top level directory, lolex.js, built by
Browserify on each release.

  1. jul. 2016 16.25 skrev "Andy Edwards" notifications@github.com:

In any case wouldn't you want lolex-src.js to work standalone in the
browser, without being built into a webpack bundle?


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#71 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAluXJNdmWLM00PPNUgOwZXYgNdFTRAFks5qV5hsgaJpZM4I93P2
.

@jedwards1211
Copy link
Contributor Author

Well I'm closing this as invalid. Sorry for the trouble

@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2016

No worries. This does show we need to update the docs, though. I see it says you need to build it yourself, which is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants