Skip to content

Support IE 11#59

Merged
theKashey merged 2 commits intotheKashey:masterfrom
happykmm:master
Nov 9, 2018
Merged

Support IE 11#59
theKashey merged 2 commits intotheKashey:masterfrom
happykmm:master

Conversation

@happykmm
Copy link
Copy Markdown
Contributor

@happykmm happykmm commented Nov 8, 2018

Add babel config to support for ie 11

@happykmm
Copy link
Copy Markdown
Contributor Author

happykmm commented Nov 8, 2018

One more thing, some of the dependencies are not IE11 compatible as well.
i.e.

  • compare-module-exports
  • wipe-node-cache
  • wipe-webpack-cache

I am thinking of 2 approaches:

  1. Apply babel transform to every of them
  2. Setup webpack for this package and let it transform all the dependencies

Any idea about this?

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 8, 2018

Codecov Report

Merging #59 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   93.73%   93.79%   +0.06%     
==========================================
  Files          85       86       +1     
  Lines        1675     1692      +17     
==========================================
+ Hits         1570     1587      +17     
  Misses        105      105
Impacted Files Coverage Δ
src/plugins.js 94.44% <0%> (-0.39%) ⬇️
_tests/lib/typed/a.js 100% <0%> (ø) ⬆️
_tests/lib/tristate/b.js 100% <0%> (ø) ⬆️
src/plugins/relative.js 100% <0%> (ø) ⬆️
_tests/scope.spec.js 100% <0%> (ø) ⬆️
_tests/tristate.spec.js 100% <0%> (ø) ⬆️
_tests/babel/babel.spec.js 100% <0%> (ø) ⬆️
_tests/lib/tristate/a2.js 100% <0%> (ø) ⬆️
src/babel/index.js 100% <0%> (ø) ⬆️
_tests/lib/tristate/a1.js 100% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a22d84a...35c626c. Read the comment docs.

@theKashey
Copy link
Copy Markdown
Owner

The easiest way - extend your local babel-loader to node_modules, as CRA does. It will just fix any possible issues.
The right way - update all libraries listed, to include build step, and become more old-browser friendly.

package.json Outdated
"build": "npm run build5 && npm run build6 && npm run buildwebpack",
"build5": "BABEL_ENV=cjs babel src -d lib && cp src/index.js.flow lib/index.js.flow",
"build6": "BABEL_ENV=ejs babel src -d es && cp src/index.js.flow es/index.js.flow",
"buildwebpack": "BABEL_ENV=cjs babel webpack -d lib-webpack",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is not quite correct way of doing it.
A bit more right way is:

  • move webpack dir to src
  • add cp lib/webpack ./webpack as buildwebpack

But I would propose just land a local .eslintrc with ES5 forced. It's literally a few lines of code to change.

Copy link
Copy Markdown
Contributor Author

@happykmm happykmm Nov 9, 2018

Choose a reason for hiding this comment

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

The issue is not only about webpack folder, but also src folder. I checked the build5 output, they do NOT have the following syntax transformed:

  • arrow functions
  • default value of function parameter

As a result, the whole package is not IE11 compatible - not just the several lines in webpack folder.
So just changing the code in webpack folder is not enough.

Copy link
Copy Markdown
Contributor Author

@happykmm happykmm Nov 9, 2018

Choose a reason for hiding this comment

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

Looks like a good idea to move webpack to src!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

changing eslint, and adding plugins to babelrc should be enought.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see. To make sure we are on the same page,

  • for webpack folder, we do not move it, instead we add eslint rc and change the code to be compatible.
  • for src folder, we add the plugins to babelrc

Do they sounds good?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yep. Sounds right.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@theKashey
Copy link
Copy Markdown
Owner

Oups! Someone already did it! #58

@theKashey theKashey merged commit e67316a into theKashey:master Nov 9, 2018
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