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: add 'optionalDependencies' to package.json #13

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

heitorado
Copy link
Contributor

@heitorado heitorado commented Apr 27, 2022

What's the purpose of this pull request?

This PR adds the optionalDependencies section at package.json and adds dependencies that are not necessary to build the application to it.
This was made so the build time is reduced, since we are now skipping unnecessary build dependencies such as eslint and storybook. A faster yarn install can now be performed with yarn install --ignore-optional

Benchmarks (at my local development machine):

Command 10 runs average Note
rm -rf node_modules && yarn install 45.7s This was the command we were running before when building the application. All packages are installed.
rm -rf node_modules && yarn install --ignore-optional 28.7s This is the new command that we intend to use in future webops builds, ignoring packages in optionalDependencies

We can observe a 37.19% reduction in the execution time of yarn install when using the --ignore-optional flag.

How does it work?

By using the optionalDependencies category, we can now only install packages that we are really going to use during build time.
This PR on webops-infra updates the gatsby build script to run yarn install with the --ignore-optional flag.

How to test it?

Basically, replicate the benchmarks mentioned in the first section. Here's a guide:

  • Clear node_modules
  • Run yarn install, with and without the --ignore-optional flag
  • Verify that the package install time decreases when using the flag
  • Run yarn build
  • Verify that the build completes successfully without the optional packages

@heitorado heitorado self-assigned this Apr 27, 2022
@vtex-sites
Copy link

vtex-sites bot commented Apr 27, 2022

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-13--gatsby.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit abbf797

@vtex-sites
Copy link

vtex-sites bot commented Apr 27, 2022

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

Page Report
/ 📎   Access the Lighthouse report here.
/apple-magic-mouse-99988212/p 📎   Access the Lighthouse report here.
/office 📎   Access the Lighthouse report here.

@igorbrasileiro igorbrasileiro requested review from tlgimenes and a team April 27, 2022 16:03
@@ -56,50 +56,52 @@
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this, but when WebOps builds for production, doesn't it already skip all the devDependencies because NODE_ENV is set to production (I'd hope)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@filipewl It does not because it has to run the tests and the lighthouse checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

But for building the static pages, I don't think they need to install the dev dependencies...

The tests and LH checks are run on their own separate pipeline run and in these cases they do install the dev dependencies to run the proper commands.

Copy link
Contributor Author

@heitorado heitorado Apr 27, 2022

Choose a reason for hiding this comment

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

@filipewl, indeed the devDependencies are not needed for building the static pages, as well as the tests and LH checks have separate pipelines.

However, if you look at our tekton dashboard and browse the pipelines' code, you'll notice that they share the same common task, called clone. This clone task is responsible for, among other things, running yarn install for the cloned repo in the CI environment. Since this task is shared between pipelines, we need to fill the optionalDependencies with the packages used in all pipelines, letting out only things we use locally, such as the storybook and eslint.

I agree that different tasks like clone-for-test and clone-for-build would be better, but:

  • yarn does not support other sections besides optionalDependencies. If they had support for testDependencies and buildDependencies that would be great.
  • The impact on the time is minimal as per the benchmarks I've run (adding cypress and lighthouse dependencies adds ~4s comparing to when only using the build dependencies)

We could investigate alternatives about to reduce the install time even further, maybe at webops level we could run different yarn install commands depending on the pipeline. But that is a story for another PR :)

@heitorado heitorado marked this pull request as draft April 27, 2022 22:47
@heitorado
Copy link
Contributor Author

Converted this to draft until webops#84 is merged and I can test the full build pipeline to evaluate which cypress and lighthouse dependencies need to be included as well.

- Move dependencies that are not necessary to build the application to 'optionalDependencies'
- A faster yarn install can now be performed with 'yarn install --ignore-optional'
@heitorado heitorado force-pushed the chore/fsss-306-add-optional-dependencies branch from ecc282f to b8f21ad Compare April 27, 2022 22:50
@vercel
Copy link

vercel bot commented Apr 27, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
gatsby-store ✅ Ready (Inspect) Visit Preview May 5, 2022 at 6:42PM (UTC)
gatsby-store-storybook ✅ Ready (Inspect) Visit Preview May 5, 2022 at 6:42PM (UTC)

@tlgimenes
Copy link
Contributor

Make sure @next/swc keeps working after these changes too

- This boolean env var allows us to choose if yarn install will be run with the '--ignore-optional' flag when running the CI
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.

4 participants