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

Use --no-detect-globals to bundle and test lolex #270

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Conversation

mantoni
Copy link
Member

@mantoni mantoni commented Oct 9, 2019

Purpose (TL;DR) - mandatory

Our own global check global || window only happened to work because browserify wrapped the whole thing with the correct check. With this fix, browserify will not define global itself.

Note that there is a very ugly hack in the top of the test file now that can / should be removed once nise and sinon received similar updates.

See mantoni/mochify.js#209 and linked issues.

Our own global check `global || window` only happened to work because
browserify wrapped the whole thing with the correct check. With this
fix, browserify will not define `global` itself.
@mantoni mantoni merged commit d3a2e31 into master Oct 10, 2019
@mantoni mantoni deleted the no-detect-globals branch October 10, 2019 07:26
@mantoni
Copy link
Member Author

mantoni commented Oct 10, 2019

Who can cut a release for this? I'm slightly unsure if this should be a patch or a minor 🤔

@mantoni
Copy link
Member Author

mantoni commented Oct 10, 2019

Released in v5.0.1.

@benadamstyles
Copy link

Hi, pretty sure this is the cause of failures I'm seeing in my build pipeline which uses lolex in puppeteer:

Error: Evaluation failed: ReferenceError: global is not defined
    at createClock (/usr/src/app/node_modules/lolex/lolex.js:959:9)
    at Object.install (/usr/src/app/node_modules/lolex/lolex.js:1219:21)

Any ideas what I should do about this? Is this something I need to handle or is this a legit bug introduced by this PR? Thanks!

@mantoni
Copy link
Member Author

mantoni commented Oct 21, 2019

@benadamstyles This is a bug, thanks for reporting! Would it be possible for you to verify that the provided patch actually works? You can copy the bundled source from jsbin here: https://jsbin.com/godesuduxo/edit?html,js,console

@mantoni
Copy link
Member Author

mantoni commented Oct 21, 2019

Fixed in v5.1.1.

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 this pull request may close these issues.

None yet

4 participants