Page MenuHomePhabricator

[Cashtab] Move extension-specific functions to conditionally loaded component
ClosedPublic

Authored by bytesofman on Dec 14 2023, 23:53.

Details

Summary

Depends on D14989

Add extension-specific logic to its own component. Load this component if env var is for building the extension.

Initially I tried to split this into several diffs, as there are multiple changes required to maintain extension functionality with only one App.js file. However, it's not practical to test these incremental changes, and each one in isolation would break either the prod app or the extension App.js.

So, this diff does "one thing" -- make the extension and the Cashtab web app build from a single App.js file, depending on the presence of an env var at build time.

To do this one thing,

If extension env var is present at build time...

  • Include extension-specific css rules with new <ExtensionFrame/> component
  • Include extension-specific address sharing logic with new <Extension/> component

If extension env var is NOT PRESENT at build time...

  • Include routes to Swap page (extension cannot support this as it relies on external code)
  • Support desktop-only easter egg
  • Include webapp service worker, <ServiceWorkerWrapper />

extension/src/App.js is deleted. The script extension.sh is modified so that only manifest.json is exchanged at build time.

For this file -- it makes sense to maintain two files because they really are totally different. chrome has manifest.json rules specific to chrome extensions (specifying things like required permission, published version number, manifest version which determines which chrome APIs are available) and react apps have their own rules (for Cashtab, this file is really just specifying the favicon and PWA icon). Changing one version of the manifest.json file does not imply any need to maintain or change the other version at the same time (unlike App.js). Arguably the build script could be optimized so we don't need to move files around at build time, but this should be done in another diff.

Test Plan

Test that webapp functionality is preserved

npm start
Confirm swap components are present

Test that extension functionality is preserved
npm run extension
Confirm the extension opens with expected dimensions
Confirm you can click the top right "open" logo and it opens in a new tab at the page that was active
Navigate to components.cashtab.com and use the "Get Address" component to confirm the address is still available from the extension

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashtab-one-app-js-one
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25970
Build 51514: Build Diffcashtab-tests
Build 51513: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Dec 15 2023, 09:19
Fabien added a subscriber: Fabien.

This diff claims to move code but it only adds code, are you missing the removal ?

This revision now requires changes to proceed.Dec 15 2023, 09:19

This diff claims to move code but it only adds code, are you missing the removal ?

It's a tricky diff to split up, because we can't really build the extension until we have all the code ready here and we deprecate extension/src/components/App.js from the extension.sh script.

I could start removing that code bit by bit, but doing this will break the extension build in the meantime. So, I think the best path is just to add what is needed here incrementally, then completely delete extension/src/components/App.js in a diff where the extension build script now works with the new approach.

The functionality of extension.js components of the app script is not directly testable until the script is complete. Could do this in one diff but imo it's easier to review in pieces, even if the test won't work until the build script is changed (which is not practical to do until all the pieces are in place).

This diff claims to move code but it only adds code, are you missing the removal ?

It's a tricky diff to split up, because we can't really build the extension until we have all the code ready here and we deprecate extension/src/components/App.js from the extension.sh script.

I could start removing that code bit by bit, but doing this will break the extension build in the meantime. So, I think the best path is just to add what is needed here incrementally, then completely delete extension/src/components/App.js in a diff where the extension build script now works with the new approach.

It's fine but this rationale should be in the summary, it's not up to the reviewer to figure this out. Also the title is misleading.

get it all working here so it's testable

bytesofman edited the test plan for this revision. (Show Details)
cashtab/src/components/App.js
483 ↗(On Diff #43709)

this could be included in the above condition block. However, it's just a coincidence that the last nav item for now is not enabled on the extension, and that this other webapp only functionality happens to be geographically next to it in the codebase. It's the kind of thing that could be messed up if either were to change, and such a change would be expected during normal Cashtab development.

cashtab/src/components/AppModes/Extension.js
1 ↗(On Diff #43709)

This code is directly copy pasted from the deleted extension/src/components/App.js

Some changes to match the syntax of "now in its own component", e.g. imports, proptypes validation, accept wallet as a prop from now-parent-component App.js

cashtab/src/components/Common/ExtensionHeader.js
1 ↗(On Diff #43709)

copy pasted from the deleted extension/src/components/App.js

This revision is now accepted and ready to land.Dec 21 2023, 10:01