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 CANARY] Add ember-decorators-polyfill dep to @ember-data/store #6321

Closed

Conversation

HeroicEric
Copy link
Member

@HeroicEric HeroicEric commented Aug 13, 2019

Extracted from #6098

#6249 refactored Store to a class. As part of this refactoring, Store#defaultAdapter was implemented using the @computed decorator. Using the @computed decorator requires Ember 3.10+.

This adds ember-decorators-polyfill as a dependency to @ember-data/store to add support for apps using an Ember version < 3.10.


Note: I don't believe this is needed for release because the conversion of Store to a class was reverted

@HeroicEric HeroicEric mentioned this pull request Aug 13, 2019
8 tasks
…/store

Extracted from emberjs#6098

emberjs#6249 refactored `Store` to a class.
As part of this refactoring, `Store#defaultAdapter` was implemented
using the `@computed` decorator. Using the `@computed` decorator
requires Ember 3.10+.

This adds [`ember-decorators-polyfill`](https://github.com/pzuraq/ember-decorators-polyfill)
as a dependency to `@ember-data/store` to add support for Ember < 3.10.
@HeroicEric HeroicEric force-pushed the add-ember-decorators-polyfill branch from 5888149 to bbb3c16 Compare August 14, 2019 00:05
@HeroicEric HeroicEric changed the title [BUGFIX beta canary] Add ember-decorators-polyfill dep to @ember-data/store [BUGFIX BETA CANARY] Add ember-decorators-polyfill dep to @ember-data/store Aug 14, 2019
@@ -26,6 +26,7 @@
"ember-cli-babel": "^7.8.0",
"ember-cli-path-utils": "^1.0.0",
"ember-cli-typescript": "^2.0.2",
"ember-decorators-polyfill": "^1.0.6",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only need this in our ember-try config, I believe it would be on consuming apps to install the polyfill? Otherwise this lgtm.

cc @rwjblue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is true, then we have to revert #6249 (since it means that [email protected] is not compatible with its own current support policy, and requires that the host app bring the polyfill).

🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue I’m more concerned with whether adding the polyfill would troll end consumers that bring their own version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @rwjblue, but lets discuss today

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw it wouldn't mean we need to revert, only that we should change from using @computed to using a getter

@runspired
Copy link
Contributor

Replaced by #6331

@runspired runspired closed this Aug 15, 2019
@HeroicEric HeroicEric deleted the add-ember-decorators-polyfill branch August 19, 2019 15:22
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.

4 participants