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

Remove transformations for older browser versions #6812

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jul 5, 2019

The Babel config had previously supported all browsers with greater than 0.25% global usage (according to browserlist). This resulted in babel-preset-env including plugins sufficient to support the following minimum browser versions:

{
  "chrome": "49",
  "android": "4.4",
  "edge": "16",
  "firefox": "52",
  "ios": "9.3",
  "safari": "11"
}

Instead, the babel config now explicitly supports chrome >= 58 and firefox >= 53. Chrome and Firefox are the only browsers we currently publish to, and these were the minimum versions with no additional Babel transformations.

The minimum browser versions we support should be re-evaluated later, when we have added tests and documentation.

The plugin 'transform-async-to-generator' has also been removed. It was used to translate async/await, but our browser targets all support async/await.

Removing some of these transformations exposed bugs in uglify-es that only presented themselves in the production build. gulp-uglify-es has been updated to a version that uses terser instead of uglify-es, which has resolved these issues.

Relates to MetaMask#6805

@Gudahtt Gudahtt requested a review from whymarrh as a code owner July 5, 2019 19:19
@Gudahtt Gudahtt closed this Jul 5, 2019
@Gudahtt Gudahtt deleted the i6805-remove-babel-transformations-for-older-browsers branch July 5, 2019 19:20
@Gudahtt Gudahtt restored the i6805-remove-babel-transformations-for-older-browsers branch July 5, 2019 19:21
@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 5, 2019

Oops... deleted the wrong branch 🤦‍♂

@Gudahtt Gudahtt reopened this Jul 5, 2019
@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 6, 2019

I've explained here why I've chosen Chrome 58 and Firefox 53.

We should probably gather more information and opinions before merging this PR. I feel pretty good about it, but if we discover a lot of users rely upon versions older than this, we might want to investigate more first before breaking the extension for them.

@danjm danjm added type-discussion DO-NOT-MERGE Pull requests that should not be merged labels Jul 8, 2019
@danjm
Copy link
Contributor

danjm commented Jul 8, 2019

Need to see if we can get more usage info regarding browser versions.

@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 9, 2019

I just left a comment on the issue describing the usage metrics I found.

tl;dr: between 0.00026%-0.047% of users would be negatively affected by the change in this PR (assuming that the metrics collected between July 1st-July 8th are representative). Given the extremely small set of affected users, merging this PR should be pretty safe even before deciding upon which browser versions to officially support.

@Gudahtt Gudahtt removed type-discussion DO-NOT-MERGE Pull requests that should not be merged labels Jul 9, 2019
@Gudahtt Gudahtt force-pushed the i6805-remove-babel-transformations-for-older-browsers branch 2 times, most recently from 6fe6f32 to 1128f00 Compare July 12, 2019 18:12
The Babel config had previously supported all browsers with greater than
0.25% global usage (according to `browserlist`). This resulted in
`babel-preset-env` including plugins sufficient to support the following
minimum browser versions:

```
{
  "chrome": "49",
  "android": "4.4",
  "edge": "16",
  "firefox": "52",
  "ios": "9.3",
  "safari": "11"
}
```

Instead, the babel config now explicitly supports chrome >= 58 and
firefox >= 53. Chrome and Firefox are the only browsers we currently
publish to, and these were the minimum versions with no additional Babel
transformations.

The minimum browser versions we support should be re-evaluated later,
when we have added tests and documentation.

The plugin 'transform-async-to-generator' has also been removed. It was
used to translate async/await, but our browser targets all support
async/await.

Removing some of these transformations exposed bugs in `uglify-es` that
only presented themselves in the production build. `gulp-uglify-es` has
been updated to a version that uses `terser` instead of `uglify-es`,
which has resolved these issues.

Relates to MetaMask#6805
@Gudahtt Gudahtt force-pushed the i6805-remove-babel-transformations-for-older-browsers branch from 1128f00 to 937903c Compare July 15, 2019 13:48
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.

I'm not sure what's the best process here but I'm definitely in favour of this

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

@kumavis
Copy link
Member

kumavis commented Jul 17, 2019

would be nice if we could display an error screen if the user's browser doesnt match the requirements
or utilize a parameter in the manifest if there is one

this could cut 0.047% * 1.1mm ~= 517 people off from their funds

@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 17, 2019

I haven't experimented with this yet, but we could set a minimum browser version in the manifest as well. That should prevent users on older browsers from installing the extension on incompatible browsers, or updating it if they already have it installed.

@whymarrh
Copy link
Contributor

[...] we could set a minimum browser version in the manifest as well. That should prevent users on older browsers from installing the extension on incompatible browsers, or updating it if they already have it installed.

If this is possible it's certainly worth doing (maybe its own PR?)

@Gudahtt
Copy link
Member Author

Gudahtt commented Jul 18, 2019

I've added the minimum browser version to the manifest in #6877

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.

4 participants