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

Update Babel Config to Support Internet Explorer #532

Merged
merged 10 commits into from
Aug 31, 2020
Merged

Conversation

colbymorrison
Copy link
Contributor

@colbymorrison colbymorrison commented Aug 29, 2020

Updates Babel and Webpack configs. They were a bit weird before so it was good we revisited them.

Previously

  • We configured babel in webpack.common.js for web and babel.config.js for testing, this was awkard.
  • For web we had babel target Node. I believe this meant that our js code was transpiled to run on Node, which is not what we want in any case, we want it to be transpiled for a browser! (Node is based on the Chrome runtime, so it worked just fine, but broke on IE as we saw)
  • We used the babel-polyfill package to add our polyfills. This copied the entire babel-polyfill package to dist/polyfill.js.

Now

  • Only configure babel in babel.config.js. If NODE_ENV == test, target Node. Otherwise, target broswerslist's "default" query. Babel now reports it is targeting these browsers:
    DeepinScreenshot_select-area_20200829124559

  • Babel-polyfill is now deprecated. So, I followed their instructions and installed corejs@3 explicitly. I added the useBuiltIns option in Babel with the usage flag. This adds polyfills as imports to the transpiled js files with the entry flag. This required placing some imports at the top of client/index.js. See the docs for a better explanation of all this. I couldn't get the entry flag to work, but it seems usage is more efficient anyway? (this confused me a bit) Now, after npm run build, there is no polyfill.js file in dist/, so dist/ is about a MB smaller since we only are using the polyfills we need, not all of them :)

-NOTE: IE11 still does not work because we are using Mbox 5+. See here and here for details, this puts us at an impass for IE support.

Internet Explorer Support

  • Downgrades Mobx to version 4, version 5 is not comparable with IE (docs). The upcoming version 6 will be apparently, so we should keep an eye out for that.
  • In addition to the changes above, we ignore the @fvilers/disable-react-devtools package in the webpack config, so it is also transpiled by Babel. It causes an error in IE without being transpiled.
  • We add the cross-fetch polyfills to client/index.js. This makes the cloud functions work in IE11. See firebase docs.

@ibeckermayer
Copy link
Contributor

It looks like the frontend tests are failing

@colbymorrison colbymorrison changed the title Update Babel Config Update Babel Config to Accomodate Internet Explorer Aug 30, 2020
@colbymorrison colbymorrison changed the title Update Babel Config to Accomodate Internet Explorer Update Babel Config to Support Internet Explorer Aug 30, 2020
Copy link
Contributor

@jcolla-holla jcolla-holla left a comment

Choose a reason for hiding this comment

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

I tested locally and functionality I tested (mostly code generation stuff) works as expected in Chrome for me (just wanted to make sure it didn't mess things up in Chrome). Approved so we can test further in staging

@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Footer renders correctly 1`] = `
<Component
<_temp2
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea why these changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I’m not sure.

Copy link
Contributor

@leon-i leon-i left a comment

Choose a reason for hiding this comment

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

It seems to be working for me - just have a few concerns:

Not sure if it's related or possibly just my internet, but I was having some trouble loading assets on the ManageMembers page. Easiest way to reproduce it was logging in -> going to settings -> then going to manage members. The table would be empty, but going to a different page and coming back would load them in. Not sure if it's a loading speed issue, or something else is going on.

Last concern is the babel message that pops up in console when running the app (see below). It seems like the fix is relatively easy though: relevant stackoverflow.

image

@colbymorrison
Copy link
Contributor Author

@leon-i

The Babel and console issues come from you using Windows. See line 17 of frontend/webpack.common.js, we're saying "don't transpile anything in node_modules except for the @fvilers package". However, this is using UNIX path syntax. So when run on Windows, the regex is interpreted as "transpile everything in node_modules", hence the Babel issues. So, you need to change Line 17 to exclude: /node_modules(?!(\/|\\)@fvilers)/. I needed to do this when testing on IE, I didn't realize one of us was using Windows to develop. Relevant webpack issue: webpack/webpack#2031.

@leon-i
Copy link
Contributor

leon-i commented Aug 30, 2020

Ah ok, makes sense - thanks for clarifying. It doesn’t seem to be impacting anything other than the console messages, so if I’m the only one it affects I don’t really mind.

@colbymorrison colbymorrison merged commit 5f591b7 into dev Aug 31, 2020
@colbymorrison colbymorrison deleted the internet-explorer branch August 31, 2020 05:42
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

4 participants