Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

Use babel-polyfill instead of phantomjs-polyfill in karma.conf #698

Closed
wants to merge 1 commit into from

Conversation

Freddy03h
Copy link

Phantom doesn't include some new method in ES6. We need to include a polyfill for some of these things in karma.conf file for running test.

Related issue : #697

@codecov-io
Copy link

Current coverage is 70.88%

Merging #698 into master will not affect coverage as of 592676f

@@            master    #698   diff @@
======================================
  Files           12      12       
  Stmts           79      79       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit             56      56       
  Partial          0       0       
  Missed          23      23       

Review entire Coverage Diff as of 592676f

Powered by Codecov. Updated on successful CI builds.

@dvdzkwsk
Copy link
Owner

dvdzkwsk commented Apr 7, 2016

Thanks for the PR. There is a note in the README with information about babel-polyfill, but perhaps it would be better to just include it. If we do end up including it, I'd like to include it in both the main bundle and the test suite so the environments match.

@dvdzkwsk
Copy link
Owner

Will be merging this, just have a few other things lined up first. 🎉

@Freddy03h
Copy link
Author

I also think we should add babel-polyfill to the webpack config for the main bundle, as you say.

And maybe add also a fetch polyfill ?

@dvdzkwsk
Copy link
Owner

dvdzkwsk commented May 5, 2016

Still considering this... I can honestly see it going either way.

@Freddy03h
Copy link
Author

Freddy03h commented May 9, 2016

Do you want me to update the PR to add babel-polyfill in the webpack config ?

This is what I already done on my current project and it work well !

@dvdzkwsk
Copy link
Owner

@Freddy03h sure, can you add it to the webpack entry point in the default bundler as well as in the karma configuration? Then we can get this merged.

@dvdzkwsk
Copy link
Owner

Implemented with fedfd79. Had to add it to the app since we were getting a bunch of issues from people not understanding problems in IE.

@dvdzkwsk dvdzkwsk closed this May 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants