-
Notifications
You must be signed in to change notification settings - Fork 33
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
Ensure polyfill works for Engines #91
Ensure polyfill works for Engines #91
Conversation
I added another commit that fixes the "prettier" errors that came up in CI. Oddly, I didn't get them locally (in my local setup, prettier tells me to do the opposite of what the CI run wanted me to do). This passes in CI at least... |
Thank you @pgengler |
}); | ||
return registry; | ||
}; | ||
for (const PolyfillTarget of [Application, Engine]) { |
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.
Sadly, I don't think we can use for of
loops (because it ends up requiring Symbol
which is not available on IE11), would you mind rewriting as either a plain for
or .forEach
?
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.
Done... rather than replace it with a different loop I just unrolled it, since it was only two elements.
This is an updated version of the changes in #48, to fix #47. Like that PR, it applies the polyfill to both Ember.Application and Ember.Engine; since Ember.Application extends from Ember.Engine this might be unnecessary and the polyfill need only be applied to Ember.Engine, but this way is unlikely to break anything. (Happy to update the PR if it's preferable to just apply to Ember.Engine.)
There aren't a lot of non-whitespace changes here; you might want to use Github's "Hide whitespace changes" feature to see the substantive changes.