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鈥檒l 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

Merged
merged 5 commits into from
Jul 23, 2021
Merged

Conversation

mansona
Copy link
Member

@mansona mansona commented May 31, 2021

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 workerpool@2.3.4 has been released our problem with Node 16 that was failing the tests has gone away 馃帀

Todo

Copy link
Contributor

@snewcomer snewcomer left a 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!

@mansona
Copy link
Member Author

mansona commented Jun 5, 2021

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.

@mansona mansona marked this pull request as ready for review June 21, 2021 21:38
@mansona mansona changed the title [WIP] Update using ember-cli-update Update using ember-cli-update Jun 21, 2021
Copy link

@sumitd94 sumitd94 left a 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>
Copy link

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>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: RE

@mansona mansona changed the title Update using ember-cli-update Update using ember-cli-update and drop support for Node 10 Jul 23, 2021
@xg-wang xg-wang merged commit d549345 into master Jul 23, 2021
@mansona mansona deleted the update branch July 23, 2021 16:25
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 this pull request may close these issues.

None yet

4 participants