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

Build failure on Node 10 #521

Closed
kentcdodds opened this issue Apr 6, 2020 · 5 comments · Fixed by #525
Closed

Build failure on Node 10 #521

kentcdodds opened this issue Apr 6, 2020 · 5 comments · Fixed by #525
Labels

Comments

@kentcdodds
Copy link
Member

As noted here: #520 (comment)

image

[test]  FAIL   node  src/__node_tests__/index.js
[test]   ● Test suite failed to run
[test] 
[test]     ReferenceError: BigInt is not defined
[test] 
[test]       at createLongLongConversion (node_modules/webidl-conversions/lib/index.js:162:51)
[test]       at Object.<anonymous> (node_modules/webidl-conversions/lib/index.js:224:24)
[test]       at Object.<anonymous> (node_modules/jsdom/lib/jsdom/browser/Window.js:3:27)
[test] 

I have no idea why that's happening. I'm guessing that we've got a devDependency that does not support Node 10. This is unfortunate and I'm kinda thinking the easiest solution is to just drop Node 10 from CI and hope we don't ever ship anything that breaks Node 10... 😬 That said, Node 10 doesn't get EOL until mid-2021 (and that's how long I think we should support it) so it may be better to find another solution to this.

Any ideas?

@eps1lon
Copy link
Member

eps1lon commented Apr 7, 2020

This looks like it originates from jsdom. Would be nice to isolate that to a minimal example just using JSDOM.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Apr 11, 2020

Looks like this commit changed webidl to use BigInt 18 days ago jsdom/webidl-conversions@d610eb6, jsdom/webidl-conversions#18

Node 10.4.0+ supports it, maybe we can just update the lts/dubnium version? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Apr 11, 2020

After some investigation in #525 (comment), I reported this upstream.

This was answered by @TimothyGu:

Let’s call this a duplicate of #2939.

I’m inclined to say that it’s your own responsibility to use the latest branch release, especially for LTS branches. Considering jsdom often uncover bugs in Node.js that get fixed in a bugfix release, I don’t know if we can ever “support” an ancient pre-LTS LTS branch release – especially not a *.0.0 release 2 years after its release. It is unfortunate that the changes slipped by without a major bump though.

I think we should bump our minimum supported Node version back to 10.18, like we did before d2885c9, so we also cover jsdom/jsdom#2939 at the same time.
Maybe make this a breaking change to be sure.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Apr 12, 2020

I think we should bump our minimum supported Node version back to 10.18, like we did before d2885c9

That commit is pretty new (Mar 13, 2020), so the number of packages that would have gotten a warning when installing between then and now, but did not because of the lowered version, should be pretty small. To actually experience an issue, they would also have had to have installed a version of JSDOM higher than 16.2.2, which was released on Jan 28, 2020. Jest 25 still installs JSDOM 15 by default. And the jest-environment-jsdom-sixteen package has Node >= 10.14 as the engine.

@kentcdodds
Copy link
Member Author

🎉 This issue has been resolved in version 7.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

4 participants