Create separate javascript bundle for notification.html

[chikeichan]

Closes #6646, #6644

Summary:

• Created NotificationRoot, NotificationRoutes, and NotificationHome component to render only the relevant components in notification popup
• Created new javascript bundle for notification.html
• Created new CSS gulp task to compile css during build vs at runtime
• Non-GZIP bundle went from 11.1MB to 8.5MB
• Evaluate Script time went from 1100ms to 925ms MBP 15" 2016, Chrome Version 74.0.3729.169

QA-ing different popup flow

[danjm]

Looks good to me except for a few small comments.

I will add e2e tests.

[danjm]

e2e tests actually pass locally. There is a yet to be diagnosed problem when building this in CI.

[danfinlay]

This is wonderful, thank you Jacky!

I guess next we should analyze why it's still 8.5MB, seems like the load time was pretty proportional to the bundle size.

I notice the notification still has a white screen at first, is there a way we could get a loading screen in during that time?

[danjm]

I notice the notification still has a white screen at first, is there a way we could get a loading screen in during that time?

That was added separately and is now in production #6648

[danfinlay]

If there were more positive reacts I'd use them.

[Gudahtt]

Looks good overall - I've tested it out a bunch locally, and I'm pretty sure it works.
I did want to more thoroughly check all possible scenarios that would result in a notification, to see whether there are unreachable paths we can get rid of in the home components and the routes. There's a bit of notification-specific logic leftover in the ui home component that we can delete now as well.

[danfinlay]

@Gudahtt a site for trying all kinds of MetaMask notifications:
https://danfinlay.github.io/js-eth-personal-sign-examples/

[Gudahtt]

Interesting that we're seeing memory failures now during test:build. I've been using this branch all week and haven't seen this happen, but now it's consistently failing every time, even locally. I suppose a recent change must have tipped it over the edge.

I'm not entirely surprised - there is more work involved in creating separate bundles, especially given all that we're duplicating. The bundle for the notification window is reduced, but the overall bundle we're generating is far bigger. Hopefully this problem can be addressed soon using code splitting - I was going to make an issue for that anyway.

For now, I've increased the heap space available to Node.js during the build:test script. That should work fine for the foreseeable future, certainly long enough to improve things. We might start seeing build failures locally though, requiring us to increase heap size available in development, which is rather inconvenient.

[Gudahtt]

This should be ready for review now. I've addressed all of my own comments and then some.

Here are the changes I've made:

• Refactor popup initialization
  The popup initialization code was duplicated between notification.js
  and ui.js. It has been moved into popup-core instead. Some changes
  were made to popup-core to improve readability as well.
• Refactor common root elements
  The Root component used by ui.js was nearly identical to
  NotificationRoot, aside from the routes it used. The common parts have
  been refactored into a separate Root component which is now used by
  both.
• Remove AccountMenu
  The AccountMenu can only be triggered by the AppHeader, which is not
  included in the notification view.
• Ensure restore-vault page is always fullscreen
  Previously the flow for restoring an account would navigate to the
  restore-vault page first, then check whether or not to open the page
  fullscreen. In the popup context, this meant an unnecessary navigation,
  as it would just close the popup anyway. In the notification context,
  this resulted in a blank page because the restore-vault screen isn't
  in the notification bundle.
  Instead it now either navigates or opens a fullscreen tab; not both.
• Add CONFIRM_ADD_SUGGESTED_TOKEN_ROUTE notification route
  This route is triggered in the notification view by the
  metamask_watchAsset method, as demonstrated here:
  https://metamask.github.io/metamask-docs/Best_Practices/Registering_Your_Token
• Ensure that initialization is always opened in browser
  These cases should currently be unreachable because of a check performed
  in the startPopup function in popup-core. There may be some edge
  case where it's possible though, so they've been updated to correctly
  open a fullscreen tab.
• Create PROVIDER_APPROVAL route
  The provider-approval page was the last one in the notification view
  without a route. It was easier to deal with all of the notification
  paths the same way, so a route has been created for this page as well.
  A redirect was required in the ProviderApproval component because of
  the possibility of there being multiple instances of the page open (e.g.
  in a notification and fullscreen). In that case, the page might get
  routed here, then the providerRequest would be removed from state.
  Redirecting is the only sensible solution in that case.
  Ideally we would handle the redirect in the action that removed the
  providerRequest in the first place, to make sure the state remained
  consistent everywhere. Unfortunately the way that the background is
  synced with the UI makes that difficult at the moment.
• Replace NotificationHome with NotificationRedirect
  The notification view doesn't really have a "home" page. Instead it's
  used for specific workflows. The NotificationHome component wasn't
  doing much more than redirect to the appropriate route.
  It has been replaced with a simpler component, with a better suited
  name.
• Refactor selectors common between routes
  Selectors for deriving the network loading status and network loading
  message have been extracted.
• Split notification-routes into container and component

[Gudahtt]

Remaining tasks:

• Profile performance impact of these changes, and take note of bundle size changes

[Gudahtt]

The notification.js bundle appears to be larger than ui.js for some reason. ui.js is 3,350,586 bytes on this branch, whereas notification.js is 3,485,520 bytes. And on develop, ui.js is 3,371,364 bytes.

Despite that, there are load time improvements for the notification page. It went from an average load time of 883.3 ms to 795.3 ms for me locally.

[Gudahtt]

This is a good change, and we do want to merge this eventually. However it does come with some downsides. It adds friction for development environments by increasing the memory required to build - you need to set NODE_OPTIONS=--max_old_space_size=3072 or it will crash. It also increases the bundle size a great deal. The benefits of this change are quite modest as well - it's maybe a 15% decrease in page load time at best.

I think the path forward here is to switch to a build system that uses tree shaking. I suspect this will completely eliminate these downsides, in that it will reduce the memory required to build and it will reduce the bundle sizes below what is currently on develop. It will also make this change much more impactful, as it'll allow the tree shaking process to remove more from each bundle, which should cut down on the bundle size enough to see much more than a 15% decrease in load time. I'm going to put this on hold until after we get tree shaking working - then this change can be resurrected.