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

Add support for IE11 #956

Merged
merged 1 commit into from
Aug 6, 2019
Merged

Add support for IE11 #956

merged 1 commit into from
Aug 6, 2019

Conversation

noisecapella
Copy link

@noisecapella noisecapella commented Aug 6, 2019

Pre-Flight checklist

  • Testing
    • Changes have been manually tested

What are the relevant tickets?

None

What's this PR do?

IE11 was not working properly because arrow functions which were present in the webpack bundle aren't supported in IE11. This PR configures webpack to also transpile libraries used by our bundle in addition to our own code. I don't know what the downsides are but I assume webpack will take longer to compile, will probably use more memory, and the bundles will probably be larger than before.

hero.js is not transpiled at all so the default parameter is removed so that it won't cause a syntax error.

Note that the hero banner video still doesn't work but this is outside the scope of this PR.

How should this be manually tested?

Go to the home page. You should be able to see the "sign in" and "create account" buttons.

What GIF best describes this PR or how it makes you feel?

@odlbot odlbot temporarily deployed to xpro-ci-pr-956 August 6, 2019 14:08 Inactive
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #956      +/-   ##
==========================================
+ Coverage   93.31%   98.22%   +4.91%     
==========================================
  Files         251      131     -120     
  Lines        8930     3672    -5258     
  Branches      506        0     -506     
==========================================
- Hits         8333     3607    -4726     
+ Misses        498       65     -433     
+ Partials       99        0      -99
Impacted Files Coverage Δ
webpack.config.shared.js 100% <ø> (ø) ⬆️
static/js/components/forms/CheckoutForm_test.js 100% <0%> (ø) ⬆️
authentication/serializers.py
mail/api.py
voucher/apps.py
courseware/admin.py
mitxpro/templatetags/render_bundle.py
mitxpro/context_processors.py
voucher/conftest.py
mitxpro/permissions.py
... and 112 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 678d634...35e0a40. Read the comment docs.

@@ -26,15 +26,13 @@ module.exports = {
},
babelSharedLoader: {
test: /\.jsx?$/,
exclude: /node_modules/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do something like this and specifically include certain node_modules files instead of including the full contents?

Copy link
Author

Choose a reason for hiding this comment

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

What files or directories would be included though? As far as I can tell it is complaining about arrow functions being referenced which could be used in any number of dependencies or libraries they depend on, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, sorry I misunderstood the root problem

Copy link
Contributor

@gsidebo gsidebo left a comment

Choose a reason for hiding this comment

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

ss 2019-08-06 at 14 33 32

This is what I'm seeing in an IE11-equipped Virtualbox VM

@noisecapella
Copy link
Author

@gsidebo By default it tries to fetch JS and CSS from localhost. You can override it with the env variable WEBPACK_DEV_SERVER_HOST to specify the IP of the laptop, at least for the purpose of testing this PR.

Copy link
Contributor

@gsidebo gsidebo left a comment

Choose a reason for hiding this comment

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

Based on the Travis builds, this looks like it's adding 2-3 minutes to the Javascript test suite build step. If there's no good way to determine which installed packages are including arrow functions and selectively include those to be transpiled, this is good to go 👍

@noisecapella noisecapella merged commit 35faa3a into master Aug 6, 2019
@noisecapella noisecapella deleted the gs/ie11 branch August 6, 2019 21:16
mbertrand pushed a commit that referenced this pull request Aug 7, 2019
@mbertrand mbertrand mentioned this pull request Aug 7, 2019
1 task
mbertrand added a commit that referenced this pull request Aug 7, 2019
This was referenced Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants