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

Fix rendering in Internet Explorer #20

Closed
ivan-aksamentov opened this issue Mar 17, 2020 · 17 comments
Closed

Fix rendering in Internet Explorer #20

ivan-aksamentov opened this issue Mar 17, 2020 · 17 comments
Labels
help wanted Extra attention is needed s:conf Scope: related to configuration s:ui Scope: related to user experience, user interface, usability, accessibility t:bug Type: bug, error
Milestone

Comments

@ivan-aksamentov
Copy link
Member

We target to support browsers starting with Internet Explorer 10, and other browsers of the same era.

Currently Internet Explorer is broken, despite us transpiling the code and having many polyfills

@ivan-aksamentov ivan-aksamentov added t:bug Type: bug, error help wanted Extra attention is needed s:conf Scope: related to configuration s:ui Scope: related to user experience, user interface, usability, accessibility labels Mar 17, 2020
@malikmukhtar
Copy link

@ivan-aksamentov Can I work on this?

@lordrip
Copy link

lordrip commented Mar 21, 2020

The problem with development is because there's some code that is not being transpiled to ES5, if you execute yarn dev and open .build/development/web/content/main.js at line 1000 you'll notice this:
const { Html5Entities } = __webpack_require__("./node_modules/html-entities/index.js");
and this is ES2015 and is not supported by our beloved IE11.

This should be fixed by editing .browserlistrc file but unfortunately I couldn't make it work yet.

@ivan-aksamentov
Copy link
Member Author

ivan-aksamentov commented Mar 22, 2020

@malikmukhtar Sure, go ahead.

@lordrip , dev stuff does not need to be compatible with IE. Neither transpilation would help. There is just too much hairy stuff injected by various dev tools. In any case, we assume that our developers are competent enough to download any of 5 latest versions of Firefox or Chrome :)

Production is transpiled differently, and mostly correctly. There are some issues with polyfills: for example my IE10 cannot find Map, even though we have a polyfill for it.

If you have some time resources, please concentrate your efforts on yarn prod:watch and opening http://localhost:8080 in IE. (for example on a Virtual Machine)

I am planing to setup browserstack bot soon(-ish)
Let me know if you need anything, folks.

@lordrip
Copy link

lordrip commented Mar 22, 2020

@ivan-aksamentov sure thing, I'll do, in order hand, and please excuse myself if this was already asked, but, CRA script shouldn't help us with all of this? I mean, developing and building for Windows (and also for IE)?.

In any case, I'll concentrate in your direction

@ivan-aksamentov
Copy link
Member Author

@lordrip we fixed development on Windows today (hopefully) in #38, towards the end.

Regarding the app not working in IE, CRA would not help at all, as every app uses different stuff and requires different polyfills. Plus, our build system have so much stuff, that changing to CRA right now would be a hiuge adventure. By the way, by CRA you mean create-react-app and their scripts package, right?)

What causesMap error, it is probably a strain library somewhere. Or maybe something tries to use Map() before polyfills are loaded. I don't know.

In any case I may be wrong, and if you find a solution, with CRA or not - submit a pull request. Our team would be very grateful!

@ivan-aksamentov
Copy link
Member Author

Related #134

@ivan-aksamentov ivan-aksamentov removed the help wanted Extra attention is needed label Mar 23, 2020
@lordrip
Copy link

lordrip commented Mar 23, 2020

Hi @ivan-aksamentov, I've migrated build part to CRA, added polyfills for IE and it works. There's still more work remaining regarding the other tools that you have in place. I'll work a little bit more on this after work and I'll share the branch to check.

@gj262
Copy link
Collaborator

gj262 commented Mar 23, 2020

Hi @lordrip, I've downloaded a VM and can double check your changes for you when/if you want.
Cheers,
-Gavin

@ivan-aksamentov ivan-aksamentov added the help wanted Extra attention is needed label Mar 24, 2020
@lordrip
Copy link

lordrip commented Mar 24, 2020

Hi @gj262 thanks a lot for the help. I've uploaded a PR here #188, but I need to check how now works to be able to deploy a preview.

In other hand, I have a warning from snyk but I'm not able to log in and check what is it all about.

In any case, I'll keep updating the PR to fix this issues.

@lordrip
Copy link

lordrip commented Mar 27, 2020

@gj262 the branch is uploaded, I think that you can test now here

🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/h7vgy31b1
✅ Preview: https://covid19-scenarios-git-fork-lordrip-cra.covid19-scenarios.now.sh

or build locally :) thanks in advance!

@gj262
Copy link
Collaborator

gj262 commented Mar 27, 2020

Hi Ricardo, @lordrip, I'll give #188 a run today. I need to repair my windows env first though. Looking @saucelabs.
It would be good to confirm that #188 fixes the issue, however the PR is quite large and touches on a lot of areas that are not related to getting IE10 working. Probably they were required to get CRA working. I'd love to see a PR that just fixes that issue. I started poking around at index.polyfilled.ts to try to understand why Map might not be loaded but my webpack is a little dated.

@lordrip
Copy link

lordrip commented Mar 27, 2020

Yes, unfortunately, since the project has TypeScript not directly together with the build, there's several situations that needs to be fixed in order to be compiled with CRA.

My reasoning behind this is since I couldn't make it work directly with polyfills, then I've tried to migrate it CRA. In any case, I understand it, I'll decline the PR and I'll be waiting in case I can help with something else on another issue.

Keep the good work and thanks for the help 💪💪💪 :)

@gj262
Copy link
Collaborator

gj262 commented Mar 27, 2020

Some progress!
0008screenshot

@gj262
Copy link
Collaborator

gj262 commented Mar 27, 2020

The Map error is caused by loading 'map.prototype.tojson' before the main entry point.

'map.prototype.tojson', // to visualize Map in Redux Dev Tools

Given that redux is not really used in this project, it is probably safe to just remove that.

There are other missing polyfills that I need to work through to get a working IE10.

@gj262
Copy link
Collaborator

gj262 commented Mar 27, 2020

Something in the tsc/babel/webpack chain is leaving/putting a 'const' in the output bundle. Don't know why. 😢
0002screenshot

@ivan-aksamentov
Copy link
Member Author

@lordrip @gj262 I am handling this. I moved redux visualizers below polyfills, so the Map problem is solved, but there are couple more errors on IE10.

We may opt-out from selective polyfilling and code-splitting and just include the whole core-js as CRA does, but I don't want to increase bundle size because of 1% of people who are still on IE. Neither I want to revamp the entire build system because of this.

@gj262
Copy link
Collaborator

gj262 commented Mar 27, 2020

I changed the polyfiller in my WIP load all core-js to get to the point above.

    typeof Object.assign === 'undefined' &&
    import(/* webpackMode: "lazy" */ /* webpackChunkName: "polyfills/core-js" */ 'core-js'), // prettier-ignore

nnoll added a commit that referenced this issue Apr 1, 2020
@ivan-aksamentov ivan-aksamentov added this to the 1.2 milestone Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed s:conf Scope: related to configuration s:ui Scope: related to user experience, user interface, usability, accessibility t:bug Type: bug, error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants