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

refactor: explicitly download electron mirror #2596

Merged
merged 29 commits into from
May 1, 2020

Conversation

karanbirsingh
Copy link
Contributor

@karanbirsingh karanbirsingh commented Apr 30, 2020

Description of changes

This PR simplifies how our build pipeline handles the mirrored electron dependency. The contents of node_modules/electron/dist and node_modules/electron-chromedriver/bin are replaced.

Before this PR:

  • mirrored electron is downloaded once in windows e2e tests and again in the electron-builder step
  • on mac/linux, e2e tests run against the original version of electron rather than the mirrored electron we're bundling with
  • e2e tests are aware of node_modules electron vs mirrored electron

After this PR:

  • mirrored electron is downloaded only once, and electron-builder is given a path to the folder
  • on all platforms, e2e tests run against the mirrored electron we're bundling with
  • e2e tests can always use the imported electron path because electron (original or mirrored) is only ever in node_modules
  • we pull in mirrored chromedriver as well

Once this PR is merged, I will fix up this codecs e2e test and put it up for review. That test is unblocked now since we download the mirrored framework separately from creating the installer. Earlier these steps were atomic so we could not hook in anywhere.

Other notes:

  • I'm unable to remove the original 'yarn install' because electron's & electron-chromedriver's install scripts handle the mirror environment variables differently. This has been fixed in master (electron-chromedriver now uses the same mechanism as electron to install), but we can't consume it yet. Once we can, then the explicit download script can go away and yarn install w/ appropriate env vars should be sufficient.
  • I switched to extract-zip instead of unzipper because on MacOS the entire file contents weren't being extracted (similar to 'close' vs 'finish' events? ZJONSSON/node-unzipper#165). extract-zip is used by electron's install script
  • sample installer here

Pull request checklist

  • Addresses an existing issue: #0000
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • (UI changes only) Added screenshots/GIFs to description above
  • (UI changes only) Verified usability with NVDA/JAWS

@karanbirsingh karanbirsingh requested a review from a team as a code owner April 30, 2020 23:44
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

Looks great overall, just a few comments

@karanbirsingh karanbirsingh requested a review from dbjorge May 1, 2020 00:33
@karanbirsingh karanbirsingh merged commit a5cc8b8 into master May 1, 2020
@karanbirsingh karanbirsingh deleted the explicit-electron-download branch May 1, 2020 14:55
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.

2 participants