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

feat: import @ember/polyfills assign conditionally #2538

Closed
wants to merge 3 commits into from

Conversation

BobrImperator
Copy link
Collaborator

@BobrImperator BobrImperator commented Apr 3, 2023

  • Uses @embroider/macros to conditionally import assign polyfill to support older and the newest ember versions.
    Should become obsolete once we decide to support only v4 and onwards where IE11 support is dropped.

Should go first #2537

Inspired by #2534

Comment on lines +5 to +10
try {
assign = importSync('@ember/polyfills').assign;
} catch (error) {
// Couldn't import @ember/polyfills
// Doesn't exist in v5
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wraps importSync in try catch becasue dependencySatisfies macro can't reliably work if the dependency isn't a direct dependency or a peer dependency.
Seems like ember-source matching in addons misbehaves especially

Copy link
Contributor

Choose a reason for hiding this comment

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

ember-source needs to be a peerDependency of this addon, I think. That should make it work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the pointer 👍 , I also realized this later on but I did get some breakage under fastboot while working on mainmatter/ember-cookies#782

@RobbieTheWagner
Copy link
Contributor

I would recommend dropping Ember.assign entirely. IE11 is not supported by Microsoft anymore anyway. Also, if someone really needs to support IE11 for some reason, it's easy to add an Object.assign polyfill and they probably already have one.

@RobbieTheWagner
Copy link
Contributor

I opened a PR to use Object.assign directly. I could add some documentation on how to add an Object.assign polyfill for IE11 if you'd like, but I think we're pretty safe not supporting IE11 at all. #2542

@BobrImperator
Copy link
Collaborator Author

BobrImperator commented Apr 15, 2023

@RobbieTheWagner Ember as a framework dropped IE11 support with v4. And I'd intend do to the same.
The problem with removing polyfills altogether is not the work needed to be done, but dropping support for older ember versions. However I agree we need to get more radical and just do it:tm: seeing how behind we are.

I'll try to resolve this in the coming days. Also I've been working on ember-cookies recently for the same reasons, since ESA uses it internally https://github.com/mainmatter/ember-cookies.
It really is just a matter of dropping older ember versions and very minimal amount of code changes.

Personally I'm also excited about the macros and resolving things this way, however mainmatter/ember-cookies#782 showed me that embroider/macros seem to not be working as I expected.
dependencySatisfies macro seems to be misbehaving over there under fastboot, with or without a peerDependency defined.

@RobbieTheWagner
Copy link
Contributor

I would argue that dropping older Ember versions isn't bad. They can always use older versions of this library. Supporting older Ember really holds a lot of addons back.

@BobrImperator BobrImperator deleted the use-macro-for-ember-polyfill branch April 21, 2023 00:12
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.

2 participants