-
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
Remove deprecated injections in Ember 4.x #900
Conversation
@@ -70,6 +70,7 @@ | |||
"ember-cli-inject-live-reload": "^2.0.2", | |||
"ember-cli-sri": "^2.1.1", | |||
"ember-cli-terser": "^4.0.1", | |||
"ember-compatibility-helpers": "^1.2.6", |
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.
@backspace this should be in dependencies
as it's used in run-time by comsumer apps rather than dev time
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.
Okay, fixed, thanks!
cf25319
to
8b0a67f
Compare
I think @mansona spend quite some time on these deprecation and has very good context here |
Thanks for your contributions. Tried installing from this branch and it broke the build.
|
@mikrostew yes I am using node 16 – thanks for pointing that out 😄 |
if (!gte('4.0.0')) { | ||
application.inject('adapter', '_ajaxRequest', 'ajax:node'); | ||
application.inject('adapter', 'fastboot', 'service:fastboot'); | ||
} |
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 don't think this check is needed, and these lines can just be removed. This would have to be released as a breaking change though.
That way apps using versions of Ember prior to 4.x would be able to use this without the implicit injections, to make the transition to 4.x easier. That's what we're running into where I work - we want to get rid of the deprecations on 3.28 before moving to 4.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.
It's also an odd behavior to have a version that both does and doesn't inject based on the Ember version. I agree that removing them and cutting a new major is the way to go
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.
So my suggestion would be to just remove the injections (and compatibility helpers that were added), and cut a new major.
@@ -1,5 +1,6 @@ | |||
/* globals najax */ | |||
import Ember from 'ember'; | |||
import { gte } from 'ember-compatibility-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.
import { gte } from 'ember-compatibility-helpers'; |
|
||
if (!gte('4.0.0')) { | ||
application.inject('adapter', '_ajaxRequest', 'ajax:node'); | ||
application.inject('adapter', 'fastboot', 'service:fastboot'); | ||
} |
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.
if (!gte('4.0.0')) { | |
application.inject('adapter', '_ajaxRequest', 'ajax:node'); | |
application.inject('adapter', 'fastboot', 'service:fastboot'); | |
} |
@@ -39,6 +39,7 @@ | |||
"ember-cli-lodash-subset": "^2.0.1", | |||
"ember-cli-preprocess-registry": "^3.3.0", | |||
"ember-cli-version-checker": "^5.1.2", | |||
"ember-compatibility-helpers": "^1.2.6", |
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.
"ember-compatibility-helpers": "^1.2.6", |
ember-compatibility-helpers@^1.2.6: | ||
version "1.2.6" | ||
resolved "https://registry.yarnpkg.com/ember-compatibility-helpers/-/ember-compatibility-helpers-1.2.6.tgz#603579ab2fb14be567ef944da3fc2d355f779cd8" | ||
integrity sha512-2UBUa5SAuPg8/kRVaiOfTwlXdeVweal1zdNPibwItrhR0IvPrXpaqwJDlEZnWKEoB+h33V0JIfiWleSG6hGkkA== | ||
dependencies: | ||
babel-plugin-debug-macros "^0.2.0" | ||
ember-cli-version-checker "^5.1.1" | ||
find-up "^5.0.0" | ||
fs-extra "^9.1.0" | ||
semver "^5.4.1" | ||
|
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.
ember-compatibility-helpers@^1.2.6: | |
version "1.2.6" | |
resolved "https://registry.yarnpkg.com/ember-compatibility-helpers/-/ember-compatibility-helpers-1.2.6.tgz#603579ab2fb14be567ef944da3fc2d355f779cd8" | |
integrity sha512-2UBUa5SAuPg8/kRVaiOfTwlXdeVweal1zdNPibwItrhR0IvPrXpaqwJDlEZnWKEoB+h33V0JIfiWleSG6hGkkA== | |
dependencies: | |
babel-plugin-debug-macros "^0.2.0" | |
ember-cli-version-checker "^5.1.1" | |
find-up "^5.0.0" | |
fs-extra "^9.1.0" | |
semver "^5.4.1" |
Thanks everyone, this work landed in #904. |
This is an attempt to fix #869. I don’t have much experience with addon development, but this seemed like a conventional way to check whether
.inject
could still be used.Thanks for the years of work on this, all!