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

Migrate to yarn for installing dependencies #8762

Merged
merged 63 commits into from Feb 18, 2021
Merged

Migrate to yarn for installing dependencies #8762

merged 63 commits into from Feb 18, 2021

Conversation

jroebu14
Copy link
Contributor

@jroebu14 jroebu14 commented Jan 20, 2021

Resolves #8761

Overall change:
Switch to using Yarn for installing node modules.

Simorgh infrastructure changes https://github.com/bbc/simorgh-infrastructure/pull/1329

Code changes:

  • replaces npm ci with yarn install --frozen-lockfile in github actions directory .github/workflows/
  • adds a preinstall script that checks yarn was used to invoke installing node modules and fails if it wasn't. inspired by https://github.com/ampproject/amphtml/blob/master/build-system/common/check-package-manager.js#L92-L97
  • removes package-lock.json and adds yarn.lock including any references in code
  • yarn doesn't have a --no-save option when installing and they are very opinionated on why. This is a problem in one of our GH actions where we install puppeteer. I've changed the action to yarn add puppeteer which by default saves to the lock file then I remove the package after running the puppeteer tests with yarn remove puppeteer. Not 100% sure if we need to do this but I suppose it cleans up the puppeteer side-effects. This might actually be needed for caching node modules in GH actions for faster build times cache node_modules in github actions #8831 since a change to the lock file will bust the cache.
  • updates snapshots for some reason

  • I have assigned myself to this PR and the corresponding issues
  • I have added the cross-team label to this PR if it requires visibility across World Service teams
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)
  • This PR requires manual testing

@jroebu14 jroebu14 added the ws-media World Service Media label Feb 3, 2021
@jroebu14 jroebu14 added this to Issue in Progress in Simorgh Feb 3, 2021
@jroebu14 jroebu14 added the cross-team For visibility for both World Service teams (Engage & Media) label Feb 3, 2021
@jroebu14 jroebu14 marked this pull request as ready for review February 3, 2021 11:25
@jroebu14 jroebu14 changed the title migrate to yarn for install dependencies Migrate to yarn for installing dependencies Feb 3, 2021
Copy link
Contributor

@andrewscfc andrewscfc left a comment

Choose a reason for hiding this comment

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

I'd be happy moving to yarn, thanks for the hard work! Just a bit concerned about the bundle size change, the bundle report may shed some light on what's going on?

.github/workflows/simorgh-local-server-tests.yml Outdated Show resolved Hide resolved
scripts/bundleSize/bundleSizeConfig.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ryanmccombe ryanmccombe left a comment

Choose a reason for hiding this comment

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

updates snapshots for some reason

😆

Copy link
Contributor

@pharingee pharingee left a comment

Choose a reason for hiding this comment

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

Brilliant!

Copy link
Contributor

@andrewscfc andrewscfc left a comment

Choose a reason for hiding this comment

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

Found a package not at latest and I've taken the liberty of updating the renovate docs

package.json Outdated
@@ -111,10 +111,10 @@
"@bbc/psammead-useful-links": "3.0.14",
"@bbc/psammead-visually-hidden-text": "2.0.3",
"@bbc/web-vitals": "1.0.2",
"@emotion/cache": "11.1.3",
"@emotion/react": "11.1.4",
"@emotion/cache": "11.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be at the latest version? Probably should test this with the latest version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must have missed this when upgrading the others 🙁

@@ -0,0 +1,19 @@
const ensureYarn = () => {
// If npm is being run, print a message and cause 'npm install' to fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice touch!

@jroebu14 jroebu14 merged commit a3bdeb3 into latest Feb 18, 2021
Simorgh automation moved this from Code Review to Done Feb 18, 2021
@jroebu14 jroebu14 deleted the migrate-to-yarn branch February 18, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team For visibility for both World Service teams (Engage & Media) ws-media World Service Media
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Migrate to Yarn
5 participants