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

Minimum Browsers Supported #4873

Merged
merged 8 commits into from
Aug 22, 2018
Merged

Conversation

ajnauleau
Copy link
Contributor

There are currently two school of thoughts around this:

"last 2 versions" and browsers that are >1%, not ie 11, not op_mini

The issue with the former are browsers such as internet explorer, as it will seize to be updated, hence holding the codebase at es2015.

The consensus of popular opinion seems to prefer the latter option, essentially supporting all browsers with a market share of above 0.25% ( http://browserl.ist/?q=%3E0.25%25%2C+not+ie+11%2C+not+op_mini+all ).

Further reads:
"last 2 versions" in babel-preset-env #7789
https://jamie.build/last-2-versions

Updated browserify in package.json and .babelrc to these standards for compatible browsers:

"browsers": [ ">0.25%", "not ie 11", "not op_mini all" ]

Seems to solve the blank white screen issue noted in #4812, and further versions of Chrome, still working on the similar fix for "MetaMask does not work with Tor browser #4771" though.

Thanks,
Antoine

@ajnauleau
Copy link
Contributor Author

Last build came out with these specs:
{ "android":"4.4", "chrome":"49", "android":"4.4", "ie":"8", "edge":"14", "ios":"9.3", "safari":"9.1", "firefox":"52", }

@whymarrh
Copy link
Contributor

Thanks @ajnauleau! I should have a chance to take a look at this early next week.

@bdresser
Copy link
Contributor

bdresser commented Aug 6, 2018

@whymarrh bumping this so we don't lose track

cc @danfinlay, bounty is yours to pay out once this is merged

@ajnauleau
Copy link
Contributor Author

@bdresser thanks for looking at this again, I honestly haven't had the time to fix the error with test-e2e-beta-firefox but it's on my list of priorities to tackle.

@danfinlay there was some mention about wanting to go further and documenting a survey of APIs and their browser compatibility. Any further thoughts on this?

@whymarrh
Copy link
Contributor

whymarrh commented Aug 6, 2018

@ajnauleau mind rebasing this on the latest develop, we've made some changes to the e2e test that should make them a bit less flaky (sorry about that!)

@ajnauleau
Copy link
Contributor Author

@whymarrh just merged latest develop, it went through the tests successfully at first but then failed on the test-e2e-beta-firefox.

Seems to be an uncaught promise rejection, nothing major, but still an issue:

UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: false == true

I can have a further look at this tomorrow.

@whymarrh
Copy link
Contributor

@ajnauleau terribly sorry about the delayed review on this! I'll take a look at this sometime this week.

(Also, don't mind the test-e2e-beta-firefox failure, we're working on making the e2e tests less flaky.)

@whymarrh
Copy link
Contributor

@ajnauleau we merged #4279 and we have .babelrc conflicts now—mind updating this PR to remove the debug flag as well (probably from both configs)? Once we've addressed the conflicts this should be good to merge!

@ajnauleau
Copy link
Contributor Author

@whymarrh I’ll jump on this, should be quick and easy— will keep you updated!

@ajnauleau
Copy link
Contributor Author

ajnauleau commented Aug 16, 2018

@whymarrh Not sure if I fully understand what you're asking. So I removed the debugging, and kept the merge version for this PR, but I'm getting an error when running gulp build now (presumably due to the minimum browser targeting).

I noticed that in the merge conflict, #4812 was being used for .babelrc, should I revert back to that then instead of merging this PR? (i.e. not targeting minimum browsers)

@ajnauleau
Copy link
Contributor Author

ajnauleau commented Aug 17, 2018

Looks like this is resolved except for the e2e tests still failing

@whymarrh
Copy link
Contributor

@ajnauleau thanks!

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM

@whymarrh whymarrh merged commit 3854650 into MetaMask:develop Aug 22, 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.

3 participants