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

chore: use hoisted yarn install in build binary #17132

Merged
merged 5 commits into from Jul 6, 2021

Conversation

tgriesser
Copy link
Member

@tgriesser tgriesser commented Jun 28, 2021

Additional details

Runs the "yarn install" from the root of the packages to de-duplicate node requires via hoisting. Reduces the overall bundle size by a considerable margin >100mb, and the compressed app by ~50mb.

Main changes:

  • Moved install path to path.join(os.tmpdir(), 'cypress-build', PLATFORM) with a symlink to build in the root for convenience (per @brian-mann request)
  • Loosen semver ranges from pinned to ^ for better deduplication of dependencies
  • Copy yarn.lock from the root before yarn --production to ensure we are building with the same dependencies we have in the lockfile for develop/testing (allows us to use the ^ rather than pinned versions with more confidence)
  • Removes additional unused directories or files demo / test that are taking up space, primarily in image libraries

How has the user experience changed?

  • Smaller bundle
  • Faster install time

Before:

Screen Shot 2021-06-28 at 2 12 53 PM

After:

Screen Shot 2021-06-28 at 2 13 03 PM

PR Tasks

  • Have tests been added/updated?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 28, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jun 28, 2021



Test summary

4125 0 50 2Flakiness 0


Run details

Project cypress
Status Passed
Commit ce28486
Started Jul 6, 2021 2:18 PM
Ended Jul 6, 2021 2:28 PM
Duration 10:22 💡
OS Linux Debian - 10.8
Browser Chrome beta 92

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@tgriesser tgriesser changed the title feat(perf): Adding root node_modules for smaller build size [WIP] feat(perf): use hoisted yarn install in build binary [WIP] Jun 28, 2021
@tgriesser tgriesser force-pushed the tgriesser/chore/root-yarn-install branch 2 times, most recently from abe7e01 to 091582b Compare June 30, 2021 22:53
@tgriesser tgriesser changed the title feat(perf): use hoisted yarn install in build binary [WIP] feat(perf): use hoisted yarn install in build binary Jun 30, 2021
@tgriesser tgriesser marked this pull request as ready for review June 30, 2021 22:54
@tgriesser tgriesser requested a review from a team as a code owner June 30, 2021 22:54
@tgriesser tgriesser requested review from kuceb and lmiller1990 and removed request for a team June 30, 2021 22:54
lmiller1990
lmiller1990 previously approved these changes Jul 1, 2021
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

I do not 100% comprehend everything that's going on here, but I gave it a test - ran the commands - and the size is indeed smaller, like Tim says in the description.

Probably good to get a second set of eyes on the code, but it looks like there's been a decent amount of internal discussion already.

@thlorenz
Copy link
Contributor

thlorenz commented Jul 1, 2021

LGTM except I have reservations with build script being TypeScript, i.e. to do the build you need to first build the scripts doing that (or at least on the fly).

I've been just adding // @ts-check on top of files involved in any kind of devops/building or whatever needed to prepare artifacts, i.e. https://github.com/thlorenz/cypress/blob/thlorenz/develop-ts/packages/ts/build-ts.js#L1.

You could go even further with that and add type annotations which are actually checked as well. See this example: https://github.com/thlorenz/cypress/blob/snapshot-8.0-release/packages/snapshot/snapconfig.js#L33-L65
Obviously only if desired, // @ts-check alone already goes a long way.

This provides sufficient warnings IMHO to not waste time discovering mistakes at runtime while keeping the build requirement to just having Node.js installed and ready.

@tgriesser
Copy link
Member Author

@thlorenz yeah it does build on the fly here with ts-node, and this script is only run when building the binary. It takes several minutes to execute (does a full production build of all modules / production yarn install / full electron bundle), so the relative overhead of transpiling on the fly is almost zero.

This provides sufficient warnings IMHO to not waste time discovering mistakes at runtime while keeping the build requirement to just having Node.js installed and ready.

Potentially, though you'd also need all of the dependencies it references installed, meaning you'd still need the yarn install

I could go either way though, I sort of think if we're trying to move more things to use .ts it's better to be consistent, but I could be convinced to keep it in .js if you still think it should be moved given the above. FWIW I think your example is potentially better suited in .js for the reasons you noted and b/c it's run more frequently.

@thlorenz
Copy link
Contributor

thlorenz commented Jul 1, 2021

@tgriesser I'm not concerned about the transpile cost, merely that now you need to have ts-node installed additionally to just Node.js.
However if we're all good with that and given we install ts-node as dev dependency then it might be ok.

It's more opinion than anything same reason why people should limit bash scripts to bash features instead of requiring zsh or similar.

If you feel .ts buys you a lot more than a ts-check could then let's agree to allow typescript in build scripts.
I might then write mine in TS as well in the future :)

@thlorenz
Copy link
Contributor

thlorenz commented Jul 1, 2021

As per discussion in Slack we agreed that TS for build scripts is fine and we should keep writing scripts in TypeScript going forward. The only exceptions are when scripts somehow execute at runtime or for our users or are very performance sensitive.

@tgriesser tgriesser changed the title feat(perf): use hoisted yarn install in build binary chore: use hoisted yarn install in build binary Jul 2, 2021
@tgriesser tgriesser requested a review from flotwig July 6, 2021 11:39
@tgriesser tgriesser force-pushed the tgriesser/chore/root-yarn-install branch from 091582b to ce28486 Compare July 6, 2021 13:58
@tgriesser tgriesser merged commit 6b3ab13 into develop Jul 6, 2021
@tgriesser tgriesser deleted the tgriesser/chore/root-yarn-install branch July 6, 2021 19:50
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.

None yet

3 participants