-
Notifications
You must be signed in to change notification settings - Fork 286
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 to ember 3.16 #1151
Update to ember 3.16 #1151
Conversation
@@ -1,5 +1,6 @@ | |||
{ | |||
"application-template-wrapper": false, | |||
"default-async-observers": 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.
Not sure if this was left out for a reason before? I just kept the blueprint default but I can flip the value if needed.
testem.js
Outdated
@@ -8,7 +8,7 @@ module.exports = { | |||
launch_in_dev: [ | |||
'Chrome' | |||
], | |||
browser_start_timeout: process.env.CI ? 60 : undefined, | |||
browser_start_timeout: process.env.CI ? 120 : undefined, |
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 kept the existing code but updated the value to match the blueprint.
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 we could probably just set it to 120 all the time, like the new blueprint does.
I also bumped the pinned node and yarn versions in separate commits. I can drop / squash them if needed. |
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.
Looks good, thanks for tackling this! I think we could make the browser timeout just 120
all the time for tests, but the rest looks good to go.
testem.js
Outdated
@@ -8,7 +8,7 @@ module.exports = { | |||
launch_in_dev: [ | |||
'Chrome' | |||
], | |||
browser_start_timeout: process.env.CI ? 60 : undefined, | |||
browser_start_timeout: process.env.CI ? 120 : undefined, |
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 we could probably just set it to 120 all the time, like the new blueprint does.
Hmm I force pushed to my branch, but that doesn't seem to update the diff view in this MR? |
I don't know why it didn't update, but I just pushed the change to |
"node": "8.17.0", | ||
"yarn": "1.19.1" | ||
"node": "10.19.0", | ||
"yarn": "1.22.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.
cc @bendemboski do you need us to cut a new major version, or is a minor version bump sufficient for you?
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 guess we need a major -- we depend on ^3.12.5
and haven't dropped node 8 support yet (and our 2.x/non-beta version also depends on ^3.12.5
), so a minor bump dropping node 8 support would break our node 8 support.
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.
Thanks for checking!
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.
LGTM, thanks! Note that this cannot be back ported to the stable branch, since it drops Node 8 support (see #1096 (comment))
* Update to node 10 * Update to Yarn v1.22.0 * Update to Ember 3.16 * Update testem.js Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
* Update to node 10 * Update to Yarn v1.22.0 * Update to Ember 3.16 * Update testem.js Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
* Update to node 10 * Update to Yarn v1.22.0 * Update to Ember 3.16 * Update testem.js Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Fixes #1148