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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Include package name in deprecation error message #20233

Merged
merged 3 commits into from
Oct 28, 2022
Merged

[BUGFIX beta] Include package name in deprecation error message #20233

merged 3 commits into from
Oct 28, 2022

Conversation

tben
Copy link
Contributor

@tben tben commented Oct 25, 2022

So this slightly changes the deprecation message to allow for package (namespace) to be included in the error message. This make it so that the message "will be removed in ember (package version)" is replaced with "will be removed in (package name) (package version)".

This change was proposed because of this usage: https://github.com/adopted-ember-addons/ember-metrics/blob/f604272de70e44ac731c711cfad5eae8792ee3ca/addon/metrics-adapters/google-tag-manager.js#L50

and this issue:
adopted-ember-addons/ember-metrics#453

So this slightly changes the deprecation message to allow for package (namespace) to be included in the error message. This make it so that the message "will be removed in ember (package version)" is replaced with "will be removed in (package name) (package version)".

This was change was proposed because of this usage:
https://github.com/adopted-ember-addons/ember-metrics/blob/f604272de70e44ac731c711cfad5eae8792ee3ca/addon/metrics-adapters/google-tag-manager.js#L50

and this issue:
adopted-ember-addons/ember-metrics#453
@bertdeblock
Copy link
Member

Seems like a nice improvement! Though AFAIK, every deprecation needs a for value, even the ones triggered by ember-source. So I don't think the 'Ember' fallback will ever be applied and you could write:

message = message + ` This will be removed in ${options.for} ${options.until}.`;

@tben
Copy link
Contributor Author

tben commented Oct 25, 2022

I did see that while changing the line. I just wanted to keep the same logic as previously set.

In theory, I could also remove some of if statement as they are a bit useless now with the asserts. However, I figured maybe they had a reason to be there.

@bertdeblock
Copy link
Member

AFAICT, 'Ember' should not have been hardcoded there in the first place.
The util is public API that can be used by any addon. Even without for being required, it would be wrong in most cases.
Therefore, I don't think it makes sense to keep the fallback.

@tben tben changed the title Include package name in deprecation function Include package name in deprecation error message Oct 26, 2022
@chriskrycho chriskrycho changed the title Include package name in deprecation error message [BUGFIX] Include package name in deprecation error message Oct 28, 2022
@chriskrycho chriskrycho merged commit 1176ed1 into emberjs:master Oct 28, 2022
@chriskrycho chriskrycho changed the title [BUGFIX] Include package name in deprecation error message [BUGFIX beta] Include package name in deprecation error message Oct 28, 2022
@tben tben deleted the patch-1 branch October 28, 2022 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants