-
-
Notifications
You must be signed in to change notification settings - Fork 259
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
Upgrade ember v3.27.0...v4.9.2 (in development) #1271
Conversation
- Ember.js v3.28 or above | ||
- Ember CLI v3.28 or above | ||
- Node.js v14 or above |
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.
These changes came from the blueprints. The peerDependencies
field currently says "ember-source": ">=3.8.0" so I imagine that would need to be updated as well.
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.
Hmmmmmm. It'd be better (I'm not sure if it's possible, but given the APIs in question I would expect it is?) to leave these untouched while moving to Ember 4.8/4.9, so that we don't have to cut a breaking change for folks to get these benefits. We do need to make a breaking change, but we'd prefer to get a last feature release out, then make the breaking change which drops support for older versions.
Also, @rwjblue may have thoughts on the version support policy; this library in particular has intentionally supported a very wide range of Ember versions, and we likely want to continue to do so… but it's possible that 3.28 is good enough.
Either way, if it's possible, let's back out these changes (and the corresponding changes to the CI matrix above) to support all of the existing supported versions unless something forces us to drop them.
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.
Update: It turns out ember-source 4.9 drops support for node 12 & 14, so we'll drop support here as well.
@@ -3,7 +3,7 @@ | |||
## Installation | |||
|
|||
* `git clone <repository-url>` | |||
* `cd @ember/test-helpers` | |||
* `cd ember-test-helpers` |
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 more closely matches what I actually had to do when installing.
@@ -18,8 +18,6 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v2 | |||
- uses: volta-cli/action@v1 | |||
with: | |||
node-version: 10.x |
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.
Volta will use node 18. If we want to use an older version of node for some of the steps I can add these back w/ 14+.
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.
We definitely need to test against Node 14, and indeed our whole supported range of Node versions, so I'm surprised we don't have a test matrix set up for that. I generally have a mild preference for running against the lowest supported version in development, simply because that's the way someone developing is least likely.
We can make the changes in a separate PR to keep this train moving, but we should:
- pin Node 14 with Volta for development
- add a matrix for Node version support, that explicitly handles 16 and 18 (since 14 will be covered "automatically")
Removing them all here and using Volta seems good, though!
config/ember-try.js
Outdated
{ | ||
name: 'ember-default-with-jquery', | ||
env: { | ||
EMBER_OPTIONAL_FEATURES: JSON.stringify({ | ||
'jquery-integration': true, | ||
}), | ||
}, | ||
npm: { | ||
devDependencies: { | ||
'@ember/jquery': '^0.6.0', | ||
'ember-fetch': null, | ||
}, | ||
}, | ||
}, |
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.
The changes in this file are straight from the blueprints. IDK what is right here.
We might still want something for jQuery this bc there does seem to be jQuery handling in the code. It has notes in it that it can be removed w/ Ember 4+ though, but I figured I should leave it since 3.28 will still be supported?
skipBabel: [ | ||
{ | ||
package: 'qunit', | ||
}, | ||
], |
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 is from the blueprints. IDK if it's right or not.
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.
Seems likely to be! 😁
package.json
Outdated
@@ -44,76 +44,76 @@ | |||
}, | |||
"dependencies": { | |||
"@ember/test-waiters": "^3.0.0", | |||
"@embroider/macros": "^1.6.0", | |||
"@embroider/util": "^1.6.0", | |||
"@embroider/macros": "^1.10.0", |
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.
Build broke w/o embroider upgrades
}, | ||
"devDependencies": { | ||
"@babel/cli": "^7.18.9", | ||
"@babel/core": "^7.20.5", |
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.
Required by peer-deps of several libraries
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 is great! I have a pair of suggested tweaks, but great forward progress!
.ember-cli
Outdated
/** | ||
Setting `isTypeScriptProject` to true will force the blueprint generators to generate TypeScript | ||
rather than JavaScript by default, when a TypeScript version of a given blueprint is available. | ||
*/ | ||
"isTypeScriptProject": true |
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.
💙
@@ -18,8 +18,6 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v2 | |||
- uses: volta-cli/action@v1 | |||
with: | |||
node-version: 10.x |
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.
We definitely need to test against Node 14, and indeed our whole supported range of Node versions, so I'm surprised we don't have a test matrix set up for that. I generally have a mild preference for running against the lowest supported version in development, simply because that's the way someone developing is least likely.
We can make the changes in a separate PR to keep this train moving, but we should:
- pin Node 14 with Volta for development
- add a matrix for Node version support, that explicitly handles 16 and 18 (since 14 will be covered "automatically")
Removing them all here and using Volta seems good, though!
- Ember.js v3.28 or above | ||
- Ember CLI v3.28 or above | ||
- Node.js v14 or above |
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.
Hmmmmmm. It'd be better (I'm not sure if it's possible, but given the APIs in question I would expect it is?) to leave these untouched while moving to Ember 4.8/4.9, so that we don't have to cut a breaking change for folks to get these benefits. We do need to make a breaking change, but we'd prefer to get a last feature release out, then make the breaking change which drops support for older versions.
Also, @rwjblue may have thoughts on the version support policy; this library in particular has intentionally supported a very wide range of Ember versions, and we likely want to continue to do so… but it's possible that 3.28 is good enough.
Either way, if it's possible, let's back out these changes (and the corresponding changes to the CI matrix above) to support all of the existing supported versions unless something forces us to drop them.
skipBabel: [ | ||
{ | ||
package: 'qunit', | ||
}, | ||
], |
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.
Seems likely to be! 😁
package.json
Outdated
"ember-resolver": "^8.0.2", | ||
"ember-source": "~3.28.4", | ||
"ember-resolver": "^8.0.3", | ||
"ember-source": "~4.9.0", |
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.
Heh, looks like we need to rename the PR to specify 4.9 instead of 4.8! 🎉
"release-it": "~14.11.6", | ||
"release-it-lerna-changelog": "^3.1.0", | ||
"rimraf": "^3.0.2", | ||
"typescript": "^4.7.4", | ||
"webpack": "^5.74.0" | ||
}, | ||
"engines": { | ||
"node": "10.* || 12.* || 14.* || 15.* || >= 16.*" | ||
"node": "14.* || 16.* || >= 18" |
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.
Unless we have to change this, we should keep it separate. Reading this set of changes to package.json
, I'm assuming the reason you ended up bumping it is because you bumped ember-cli
as well as the others, which in turn requires it for more recent versions IIRC? If so, try reverting to the previous version of ember-cli
, which should be forward compatible with these versions of ember-source
etc., and that will allow us to make the move I described above in terms of versioning/release strategy.
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.
I think it's coming from ember-source as well.
ember-source@4.9.0: The engine "node" is incompatible with this module. Expected version ">= 14.*". Got "12.22.4"
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.
For posterity: I made the change, which landed in Ember 4.9, to drop support for older versions of Node in Ember itself… and had forgotten it when I made this comment.
What I think I’m going to do is cut a release of the current major version of @ember/test-helpers
which does not include this particular update, but does ship types. That'll do what we intended to do months ago (though apparently no one has been depending on that, since we haven't gotten bug reports!). At that point I'll also update DefinitelyTyped to match.
Then this PR, and shipping native Ember types, can go into a new major version.
UPDATE: Resolved the issue below. It was related to an issue in my dev environment. Phew. There might be an issue somewhere. I am trying to use this version locally and getting:
Update: This might be a problem on master |
@gitKrystan I'm going to re-review the changes now, but we'll need to update the CI config in |
Eh I tried to help here but it looks like it needs to be rebased. |
69ae2c9
to
27d4fe4
Compare
Rebased |
Looks like the embroider-safe |
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.
Couple of tiny tweaks here, and then let's land it!
"release-it": "~14.11.6", | ||
"release-it-lerna-changelog": "^3.1.0", | ||
"rimraf": "^3.0.2", | ||
"typescript": "^4.7.4", | ||
"webpack": "^5.74.0" | ||
}, | ||
"engines": { | ||
"node": "10.* || 12.* || 14.* || 15.* || >= 16.*" | ||
"node": "14.* || 16.* || >= 18" |
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.
For posterity: I made the change, which landed in Ember 4.9, to drop support for older versions of Node in Ember itself… and had forgotten it when I made this comment.
What I think I’m going to do is cut a release of the current major version of @ember/test-helpers
which does not include this particular update, but does ship types. That'll do what we intended to do months ago (though apparently no one has been depending on that, since we haven't gotten bug reports!). At that point I'll also update DefinitelyTyped to match.
Then this PR, and shipping native Ember types, can go into a new major version.
}, | ||
}, | ||
}, | ||
{ | ||
name: 'ember-lts-3.28', | ||
name: 'ember-lts-4.4', |
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.
As noted above, as of this week, we also need an ember-lts-4.8
scenario.
7e7d16a
to
680ea54
Compare
Remove old versions of ember
55fff90
to
c6f097d
Compare
@chriskrycho Rebased |
Needs to test 4.8, not just 4.4 twice! 😄 |
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.
Let's do it! 🎉 🚀
The `concurrently` invocation was set to run all `ember-try` scenarios *every* time we run `yarn test` either locally or in CI, rather than only when we opt into doing so with `test:all`. Switch back to doing a single `ember test` invocation as the main `test` invocation instead.
We want this on at *some* point, but not as part of this PR.
This will give us access to the preview types, which will help with emberjs/ember.js#20254
Read carefully because I guessed on a lot of things. 🤠