-
Notifications
You must be signed in to change notification settings - Fork 159
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
Update using ember-cli-update and drop support for Node 10 #834
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like great work!
Thanks for the note @snewcomer 😂 but I don't entirely agree that this looks like great work 😂 The thing is I don't know that putting a resolution in our package.json will "fix" the issue, because anyone downstream will still have this issue. I briefly discussed this with @Turbo87 in discord the other day and we had some ideas on how to proceed: https://discord.com/channels/480462759797063690/485861149821239298/850047640816582767 I would love to know your thoughts, It's a shame I couldn't join the meeting yesterday because I would have loved to discuss this. |
https://nodejs.org/en/about/releases/ # Conflicts: # packages/ember-cli-fastboot/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two NIT Comments. This looks great 🎉 🔥
@@ -21,6 +21,13 @@ | |||
{{content-for "body"}} | |||
{{content-for "test-body"}} | |||
|
|||
<div id="qunit"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: we can self enclose the tag here
<div id="quint" />
<div id="qunit"></div> | ||
<div id="qunit-fixture"> | ||
<div id="ember-testing-container"> | ||
<div id="ember-testing"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: RE
So this PR has been a bit of a saga 🙈
Essentially I have been trying to get #824 working (so that we can add node 16 to our test suite) and I have been having the world of trouble.
To make a long story very short: we had a problem with a sub-sub-sub-dependency of ember-cli-babel@6, workerpool. The version that we had in our sub-sub-sub-dependencies was very very old and had a bug in it that broke Node 16 that was fixed in the latest version.
My first attempt to fix this was to try and update the version of workerpool that we used in our old ember-cli-fastboot and potentially release a patch version of that: babel/broccoli-babel-transpiler#203 but @rwjblue rightly said that this caused odd semver concerns.
in that conversation @stefanpenner said that if we could patch the old version of workerpool then we could release a patch of that old version, so I did 🎉 josdejong/workerpool#309
Now that [email protected] has been released our problem with Node 16 that was failing the tests has gone away 🎉
Todo