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

remove polyfill #52

Merged
merged 2 commits into from
Aug 19, 2016
Merged

Conversation

ianstarz
Copy link
Contributor

@ianstarz ianstarz commented Aug 19, 2016

PATCH

All tests pass locally without this. Would love to not include the polyfill if possible.

CHANGELOG

  • Only import polyfill for CI and not for consuming apps. Apps that want Symbol can pay as they go.

@sandersky
Copy link
Contributor

sandersky commented Aug 19, 2016

This is necessary to pass on Travis with the version of Firefox it is using.

@ianstarz
Copy link
Contributor Author

@sandersky can we have it switch on CI environment flags? importing the browser-polyfill.js is an expensive cost for apps that don't need it

@sandersky
Copy link
Contributor

sandersky commented Aug 19, 2016

@ianstarz if you want you can probably just move this out of index.js and into ember-cli-build.js. Then this app should pass CI and any consumers using Symbol() can simply enable the polyfill in their ember-cli-build.js if need be.

@sandersky
Copy link
Contributor

sandersky commented Aug 19, 2016

👍 Thanks for for the pull request @ianstarz. Glad to see my work being used and contributed to.

Approved with PullApprove

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Changes Unknown when pulling 323f839 on ianstarz:remove-polyfill into * on ciena-blueplanet:master*.

@sandersky sandersky merged commit 630976d into ciena-blueplanet:master Aug 19, 2016
@ianstarz
Copy link
Contributor Author

@sandersky no problem! I'm glad it was an easy fix, this is a great solution to some problems we have.

@ianstarz
Copy link
Contributor Author

ianstarz commented Aug 19, 2016

@sandersky I forgot to bump version; do you mind doing that and publishing to npm?

@sandersky
Copy link
Contributor

sandersky commented Aug 19, 2016

@ianstarz the CI handles that for us. You will notice I added #PATCH# to your PR description which tells the CI to make a patch bump. You can learn more about it here. Sometimes the CI is a little slow though so give it some time to actually run the merge build and publish to npm.

@ianstarz
Copy link
Contributor Author

OOOOoooooo!! Thanks, very cool!

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.

None yet

3 participants