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

Merge MetaMask/develop #32

Closed
wants to merge 140 commits into from
Closed

Merge MetaMask/develop #32

wants to merge 140 commits into from

Conversation

@cg505
Copy link
Contributor

cg505 commented Jul 29, 2019

There's not necessarily any reason to do this right now, but it does upgrade to node 10.

Please check out the very last commit, which includes a fix to make style loading work correctly after the merge.

tmashuang and others added 30 commits Apr 8, 2019
Merge develop into master
Update master branch with develop (v6.4.0)
Update master with 6.4.1 Release
Master Version Bump (v6.5.0)
Update master for 6.5.2 production release
Merge dev to master
Develop to master
Version 6.6.2
Gudahtt and others added 23 commits Jul 18, 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 #6805
* Add some notes

* Add explanatory comment and TODO

* Typo

* Improve verbage

* Remove contextual comment
The function `chrome.extension.getURL` has been deprecated since Chrome
58 [1]. It is completely equivalent to `chrome.runtime.getURL`, which
has been around since Chrome 31.

[1]: https://developer.chrome.com/extensions/extension#method-getURL
The CSS is now served as an external file instead of being injected.
This was done to improve performance. Ideally we would come to a middle
ground between this and the former behaviour by injecting only the CSS
that was required for the initial page load, then lazily loading the
rest. However that change would be more complex. The hope was that
making all CSS external would at least be a slight improvement.

Performance metrics were collected before and after this change to
determine whether this change actually helped. The metrics collected
were the timing events provided by Chrome DevTools:

* DOM Content Loaded (DCL) [1]
* Load (L) [2]
* First Paint (FP) [3]
* First Contentful Paint (FCP) [3]
* First Meaningful Paint (FMP) [3]

Here are the results (units in milliseconds):

Injected CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1569.45 | 1570.97 | 1700.36 | 1700.36 | 1700.36 |
| 2 | 1517.37 | 1518.84 | 1630.98 | 1630.98 | 1630.98 |
| 3 | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| 4 | 1522.15 | 1523.72 | 1629.3 | 1629.3 | 1629.3 |
| **Min** | 1517.37 | 1518.84 | 1629.3 | 1629.3 | 1629.3 |
| **Max** | 1603.71 | 1605.31 | 1712.56 | 1712.56 | 1712.56 |
| **Mean** | 1553.17 | 1554.71 | 1668.3 | 1668.3 | 1668.3 |
| **Std. dev.** | 33.41 | 33.43 | 38.16 | 38.16 | 38.16 |

External CSS:

| Run | DCL | L | FP | FCP | FMP |
| :--- | ---: | ---: | ---: | ---: | ---: |
| 1 | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| 2 | 1537.55 | 1538.99 | 199.38 | 1633.5 | 1633.5 |
| 3 | 1571.28 | 1572.74 | 268.65 | 1677.03 | 1677.03 |
| 4 | 1510.98 | 1512.33 | 206.72 | 1607.03 | 1607.03 |
| **Min** | 1510.98 | 1512.33 | 199.38 | 1607.03 | 1607.03 |
| **Max** | 1595.4 | 1598.91 | 284.97 | 1712.86 | 1712.86 |
| **Mean** | 1553.8025 | 1555.7425 | 239.93 | 1657.605 | 1657.605 |
| **Std. dev.** | 29.5375 | 30.0825 | 36.88 | 37.34 | 37.34 |

Unfortunately, using an external CSS file made no discernible improvement
to the overall page load time. DCM and L were practically identical, and
FCP and FMP were marginally better (well within error margins).

However, the first paint time was _dramatically_ improved. This change
seems worthwhile for the first paint time improvement alone. It also
allows us to delete some code and remove a dependency.

The old `css.js` module included two third-party CSS files as well, so
those have been imported into the main Sass file. This was easier than
bundling them in the gulpfile.

The resulting CSS bundle needs to be served from the root because we're
using a few `@include` rules that make this assumption. We could move
this under `/css/` if desired, but we'd need to update each of these
`@include` rules.

Relates to #6646

[1]: https://developer.mozilla.org/en-US/docs/Web/Events/DOMContentLoaded
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/load
[3]: https://developers.google.com/web/fundamentals/performance/user-centric-performance-metrics
* Bump eth-sig-util to 2.3.0

* Bump eth-keyring-controller to 4.x
* Resolve onion address
* npm i content-hash@latest
* Disable open dropdown for single asset

* add test to check single asset in dropdown

* fix lint errors, remove unused import
The npm audit script was auditing all dependencies, then filtering the
results to just the advisories concerning production dependencies. This
was done by checking the boolean `dev` and `optional` properties of each
`findings` entry in each advisory.

The `dev` and `optional` properties are now missing, which is resulting
in dev advisories being mistakenly identified as affecting production.

This check has been removed, and instead the `--production` flag is used
when calling `npm audit`. This accomplishes the same goal without
relying as much upon the audit output format.

The `--production` flag was added in `npm` `v6.10.0`, so `npm` has been
updated to the current latest stable (`v6.10.2`) for the `test-deps`
job. It was also updated on the `prep-deps-npm` job to ensure
consistency in behaviour. The other jobs only use `npm run` which hasn't
changed substantially in some time, so compatibility isn't really a
concern for those.

`audit.json` has also been added to `.gitignore`. It was accidentally
checked in once while working on this branch.
This callback has been unused for a long time. It was removed in #1076
The `time-remaining` component hasn't been used since #5704, aside from
a few styles. Those styles have been integrated into the
`advanced-tab-content` styles, and the unused component has been
deleted.
The `AccountDropdownMini` component featured the ability to switch
accounts using a dropdown, but this functionality was disabled in #6024.
It has been acting as a restyled `AccountListItem` since then.

The component has been removed, and the style changes moved to the sole
parent component (`RequestSignature`).
The `seedPhrase` prop has not been passed into the
`first-time-flow-switch` component since #5994.
`test-deps` runs `npm audit`, which doesn't require the dependencies to
be installed. `npm audit` just uses the lockfile.
These constants were not referenced outside of this file
`seedWords` used to be stored on the metamask state temporarily at
certain points. This hasn't been the case since #5994, but references
to this state remained. All of the logic remained for correctly updating
these `seedWords`, handling them during navigation, and scrubbing them
from the state.

However the state was never updated in practice. The `seedWords` are
still returned by `verifySeedPhrase`, and they're still stored in
component state in a few places. But they aren't ever set in the Redux
metadata state or the Preferences controller.

All references to this state have been removed, along with any logic
for interacting with this state. A few unused actions were removed as
well.
The `accounts` prop of `SignatureRequest` was throwing a PropType
warning because `accounts` was an object instead of an array. It looks
like when the `mergeProps` function was added in #6340, the ownProps
were accidentally set to override the state props.

The now ignored props have been removed from the parent `ConfirmTxScreen`
component as well. `conversionRate` was identical to the one retrieved
in `SignatureRequest`, and `selectedAddress` differed only in the
fallback behaviour when `state.metamask.selectedAddress` does not exist;
it will now default to the first account instead (as was the original
behavior, prior to #6340).
These props were discovered to be unused while I was working on #6680
This creates a more robust method for injecting our SCSS imports. The
old method was broken by the merge, because the last line of the
modified file changes. This method does not rely on any knowledge of
the file.
@cg505 cg505 requested review from ryanml and bbondy Jul 29, 2019
@bbondy
Copy link
Member

bbondy commented Jul 30, 2019

I prefer we rebase on top for now

@bbondy
Copy link
Member

bbondy commented Jul 30, 2019

Thanks for doing the PR though!

@cg505
Copy link
Contributor Author

cg505 commented Jul 30, 2019

I'll cherry-pick the load-styles commit to a separate PR.

@cg505 cg505 closed this Jul 30, 2019
@cg505 cg505 deleted the merge-upstream branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.