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

fix: update docs around pluralizedTypes and type the field #951

Merged
merged 1 commit into from
Aug 15, 2023
Merged

fix: update docs around pluralizedTypes and type the field #951

merged 1 commit into from
Aug 15, 2023

Conversation

SergeAstapov
Copy link
Contributor

@SergeAstapov SergeAstapov commented Aug 11, 2023

In #902 resolver was converted to native class which has a different semantics around object initialization.

Per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Public_class_fields

Public instance fields are added to the instance either at construction time in the base class (before the constructor body runs), or just after super() returns in a subclass.

If you define pluralizedTypes in the app, previously the following lines run in init() hook,which runs after properties assigned to the class instance in the child class.

if (!this.pluralizedTypes.config) {
  this.pluralizedTypes.config = 'config';
}

now, above lines run before properties assigned to the class instance in the child class. Which means, pluralizedTypes defined in the child class overrides config mapping.

Because of this semantic change, any addon doing application.resolveRegistration('config:environment') would receive undefined instead of reference to the app config.

Docs should anyway teach updated syntax of pluralizedTypesdefinition.

We may argue if addons doing the right thing here or not (I would think latter is true as addons should migrate to @embroider/macros and utilize getOwnConfig, getConfig, and getGlobalConfig), but that's out of scope of this issue.

EDIT: no tests added here as there is actually nothing wrong with the addon code, it's just an app code needs to be updated.

@ef4 ef4 merged commit c651f3a into ember-cli:main Aug 15, 2023
10 checks passed
@SergeAstapov SergeAstapov deleted the pluralized-types-native-class-fix branch August 15, 2023 15:44
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.

3 participants