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

IE11: Update babel config to use the runtime #404

Closed
wants to merge 1 commit into from

Conversation

oigewan
Copy link

@oigewan oigewan commented Jun 14, 2017

Bug:
Fixes #403

As mentioned in the issue, this library breaks in IE11 due to missing polyfills. You could just document the polyfill requirements for this library, but since IE 11 is still over 2% of browser usage and this is a library, I think it'd be a good idea to set up babel to use polyfills and helpers.

I also had issues testing changes to the library. When I ran npm link to test my changes, I kept getting errors saying that webpack could not find the "react" library. I looked and saw that React is listed as a peerDependency. However, since React is imported by the library's code and is required in order for this library to compile properly, I'm thinking it should be a dependency.

Notes:
Babel documentation on the plugins I'm using and dependencies I added.

Changes:

  • Add the transform-runtime plugin configured to use the helpers and polyfills.
  • Add a dependency on the babel-runtime.
  • Moved React from a peerDependency to a dependency.

cc: @gpbl

Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for helping with this! I've added some comments to your changes.

@@ -63,9 +63,6 @@
"url": "https://github.com/gpbl/react-day-picker/issues"
},
"homepage": "https://react-day-picker.js.org",
"peerDependencies": {
"react": "~0.13.x || ~0.14.x || ^15.0.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why you are removing react from peerDependencies?

Copy link
Author

Choose a reason for hiding this comment

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

Mentioned in the PR description, but I'll copy/paste here:

I also had issues testing changes to the library. When I ran npm link to test my changes, I kept getting errors saying that webpack could not find the "react" library. I looked and saw that React is listed as a peerDependency. However, since React is imported by the library's code and is required in order for this library to compile properly, I'm thinking it should be a dependency.

Copy link
Owner

@gpbl gpbl Jun 15, 2017

Choose a reason for hiding this comment

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

Sorry I missed that part 😄

React is included in the devDependencies, which should be installed when developing the module. So definitely react must stay as peerDependency here.

@@ -113,7 +111,9 @@
"webpack": "^2.6.1"
},
"dependencies": {
"prop-types": "^15.5.10"
"babel-runtime": "^6.23.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Do you know the impact this is having to the final bundle size? I'd rather remove the stuff not supported by IE11.

Copy link
Author

Choose a reason for hiding this comment

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

The bundle size did go up a noticeable amount. I did some investigation on what would need to be done. The Array.from references are actually introduced by a babel plugin, so it's affects aren't quite easy to see.

I'll close this PR and open another one as soon as I'm capable of testing my changes.

@gpbl gpbl changed the title IE11 Supoort: Update babel config to use the runtime IE11: Update babel config to use the runtime Jun 15, 2017
@oigewan oigewan closed this Jun 15, 2017
@gpbl
Copy link
Owner

gpbl commented Jun 15, 2017

@oigewan

  • These lines in Day.js could be use the object-assign module (to add as dependency).
  • I'm still wondering where does this Array.from come from 🤔

@gpbl
Copy link
Owner

gpbl commented Jun 15, 2017

@oigewan ah the Array.from comes from spreading arrays, e.g. here. We can also fix that without adding other dependencies...

@oigewan
Copy link
Author

oigewan commented Jun 15, 2017

Yup. New PR: #406

@oigewan oigewan deleted the babel branch June 20, 2017 03:36
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.

2 participants