refactor(package): Migrate from npm to Yarn by vojtechszocs · Pull Request #289 · patternfly/patternfly-react

vojtechszocs · 2018-03-27T14:36:27Z

Fixes issue #215.

$ node -v
v8.9.4
$ npm -v
5.6.0
$ yarn -v
1.5.1

npm => Yarn migration details are outlined below.

1, Populate node_modules via npm

$ rm -rf ./node_modules
$ npm install

2, Generate initial yarn.lock from package-lock.json via synp

$ synp --source-file ./package-lock.json

3, Project-specific fixes

package.json

npm run => yarn run

storybook/webpack.config.js

reuse sassIncludes from package.json (avoid duplication)

These fixes reflect the difference between npm-generated vs. Yarn-generated node_modules file structure:

npm - e.g. node_modules/patternfly/node_modules/font-awesome-sass (nested structure)
Yarn - e.g. node_modules/font-awesome-sass (flat structure)

4, Update yarn.lock and fix package checksums if necessary

$ rm -rf ./node_modules
$ yarn install --update-checksums

5, Verification

$ rm -rf ./node_modules
$ yarn install

No changes in yarn.lock - all is good.

$ yarn run storybook:run

Storybook components look and work as expected.

vojtechszocs · 2018-03-27T15:01:49Z

I'd like to share some thoughts on why using Yarn instead of npm is a good thing, at least for the time being:

For isolated CI environments (without internet access), Yarn has an offline mirror feature that allows you to populate node_modules of your project from a local mirror of npm registry. In oVirt project, we use this via ovirt-engine-nodejs-modules. I'm not aware of an equivalent feature in npm.
npm has a very "bumpy" history: from optional dependencies causing unexpected package-lock.json changes to bricking your whole system by changing critical Linux filesystem permissions. Some bigger projects adopt Yarn, like VS Code.
npm doesn't seem to have that much outside contributions: "Not a single pull request was merged in the last 2 months that came from an outside contributor. There are currently over 70 PRs open and none of them have any activity from the npm team. EDIT: Last merged PR from an outsider was back in November.".

Last but not least, it was Yarn who pioneered the idea of version locking and package checksums, which speeded up adoption of those concepts in npm.

vojtechszocs · 2018-03-27T15:02:54Z

cc @priley86 @jeff-phillips-18 @mturley @karelhala

AllenBW · 2018-03-27T15:10:51Z

package.json

  },
  "scripts": {
-    "start": "concurrently \"npm run storybook:run\" \"npm run storybook:openurl\"",
+    "start": "concurrently \"yarn run storybook:run\" \"yarn run storybook:openurl\"",


With yarn, you don't need to have the run command to execute the script, can just call it by its name yarn storybook:run

You are right, I'll use the shortcut as suggested.

I tend to prefer an explicit yarn run since CLI docs say:

Note that built-in cli commands will have preference over your scripts, so you shouldn’t always rely on this shortcut in other scripts

but for stuff inside the scripts object, it should be fine.

karelhala · 2018-03-27T16:36:03Z

storybook/webpack.config.js

-                  __dirname,
-                  '../node_modules/patternfly/node_modules/font-awesome-sass/assets/stylesheets'
-                )
+                path.resolve(__dirname, `../${pkg.sassIncludes.patternfly}`),


I really like this approach, but this can also be simplified to

Object.keys(pkg.sassIncludes) .map(oneInclude => path.resolve(__dirname, `../${pkg.sassIncludes[oneInclude]}`))

Thanks Karel, I agree - will do the code change as suggested.

path.resolve(__dirname, '../sass/patternfly-react'), - path.resolve(__dirname, `../${pkg.sassIncludes.patternfly}`), - path.resolve(__dirname, `../${pkg.sassIncludes.bootstrap}`), - path.resolve(__dirname, `../${pkg.sassIncludes.fontAwesome}`) + ...Object.values(pkg.sassIncludes).map(includePath => + path.resolve(__dirname, `../${includePath}`) + )

karelhala · 2018-03-27T16:36:36Z

Looks good, I really like how you solved the import of sass files.

priley86 · 2018-04-02T14:50:46Z

travis is not yet happy... otherwise it looks good to me...

jeff-phillips-18 · 2018-04-03T16:40:21Z

Need to update the Readme, otherwise LGTM.

vojtechszocs · 2018-04-05T18:08:29Z

Addressed Allen's and Karel's review comments, still need to update the README file.

vojtechszocs · 2018-04-05T18:33:25Z

README file updated.

Note that with Yarn 1.0 and above, this command:

npm run foo -- extra

is equivalent to this command:

yarn foo extra

i.e. no need for -- to provide additional script parameters. (If you do, Yarn emits a warning, saying that in future, -- will be forwarded to the script as-is, so we're guarding against that.)

vojtechszocs · 2018-04-05T18:45:16Z

yarn install, yarn build and yarn storybook:run commands work as expected. Storybook app looks & feels as before.

Here's a checklist of things to do before merging:

update dependencies based on current master, namely chore(dependencies): Update non-breaking deps #293 and Docs(storybook): update to storybook 3.4 #269
ensure Travis CI is passing (might need to delete master branch cache)

priley86 · 2018-04-05T20:49:49Z

ensure Travis CI is passing (might need to delete master branch cache)

cleared all of them just now.

vojtechszocs · 2018-04-09T16:48:26Z

cleared all of them just now

Thank you @priley86.

I'm trying to use a modified version of patternfly-react (contains my VM provision wizard component) in another project which uses Yarn and realized that yarn link only works when both projects use Yarn.

I'll update this PR (regenerate yarn.lock) and rebase my local modifications on top of this PR. I hope we can merge this PR soon 😃

vojtechszocs · 2018-04-09T16:52:41Z

realized that yarn link only works when both projects use Yarn

Actually, I was wrong. yarn link (adding a local package to Yarn's link registry) seems to work even if the given project (e.g. patternfly-react) is managed by npm.

success Registered "patternfly-react".
info You can now run `yarn link "patternfly-react"` in the projects where you want to use this module and it will be used instead.

vojtechszocs · 2018-04-09T18:38:40Z

Oh well, looks like that even if yarn link succeeds on npm-managed project, Yarn-managed project build fails due to node_modules nested (npm) vs flat (Yarn) structure.

priley86 · 2018-04-09T19:54:21Z

I am fine w/ removing the package.lock in favor of yarn.lock. Let me know if you need anything else from me...

vojtechszocs · 2018-04-11T17:51:05Z

PR updated to reflect recent changes in package.json and package-lock.json.

yarn install, yarn build and yarn storybook:run commands work as expected. Storybook app looks & feels as before.

✔️ checked again and everything works as expected.

vojtechszocs · 2018-04-11T17:55:38Z

I am fine w/ removing the package.lock in favor of yarn.lock. Let me know if you need anything else from me...

@priley86 thanks, this PR should be ready to go, assuming Travis CI passes.

package-lock.json was removed, it doesn't make much sense to have both yarn.lock and package-lock.json files - these would have to be kept in sync, which is (in my opinion) not worth the trouble.

vojtechszocs · 2018-04-12T10:20:21Z

OK, just realized Travis CI fails because .travis.yml and release.sh still use npm-based commands. I'll fix this.

coveralls · 2018-04-12T11:06:14Z

Pull Request Test Coverage Report for Build 1157

0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 72.499%

Totals
Change from base Build 1127:	0.0%
Covered Lines:	1259
Relevant Lines:	1579

💛 - Coveralls

vojtechszocs · 2018-04-12T12:39:49Z

Travis CI build is fixed.

I also made some changes to .travis.yml according to Travis docs:

1, Caching

Using

cache:
  directories:
    - "node_modules"

applies to (or used to apply to) npm, since npm traditionally didn't have a local package cache.

Yarn came with this concept right from the start, in Yarn's terminology it's called "global cache". You can use yarn cache dir to print its location, e.g. ~/.cache/yarn. You can use yarn cache clean to clear the cache. Details are here.

In response to Yarn, npm introduced the same concept, in npm's terminology it's called "npm cache". Cache location is typically ~/.npm. You can use npm cache clean to clear the cache. Details are here.

Today, with each of those package managers working with a local package cache, it doesn't make much sense to cache (project-specific) node_modules directory.

For Yarn specifically, Travis supports

cache: yarn

that causes everything under ~/.cache/yarn to be cached across Travis builds.

2, Node.js version

You can specify explicit versions like

node_js:
- '8'
- '6'

however this causes Travis to skip reading the .nvmrc file. For consistency, it's much better to simply remove the node_js declaration, which makes Travis read .nvmrc file and use the Node.js version specified in it.

3, Pruning unused packages in `node_modules`

Right now, there is

before_script:
- npm prune

but it doesn't make much sense from Yarn perspective. yarn prune isn't necessary since yarn install automatically prunes extraneous packages. Therefore, I removed above mentioned before_script declaration.

priley86 · 2018-04-12T13:23:52Z

this looks good to me... and thanks a lot for the yarn notes. Do you mind updating the first commit to be semantic? Since you aren't really impacting downstreams and it is build specific here i think it should be chore(), but let us know if you disagree... Also - it seems that Node 6 can be removed now...

vojtechszocs · 2018-04-12T17:22:36Z

Thank you @jeff-phillips-18 and @priley86 for reviewing.

Do you mind updating the first commit to be semantic?

Will do 😃

Since you aren't really impacting downstreams and it is build specific here i think it should be chore(), but let us know if you disagree...

I've checked this PR by yarn link-ing local patternfly-react copy (containing commits from this PR) with oVirt Dashboard project and using Modal component containing buttons, icons and some other stuff. Everything looks and works as expected.

Is it OK to use refactor commit type here?

refactor(package): Migrate from npm to Yarn

Due to the size of package-lock.json (removed 21,093 LOC) vs. yarn.lock (added 11,133 LOC) it's not humanly possible to compare these two files. synp didn't output anything during conversion, so I'm assuming everything is OK, but still, to be on safe side, I'd rather use refactor instead of chore because the lock file does impact the build output.

What do you think? (Ultimately, I'm OK with chore too, if you prefer that.)

Also - it seems that Node 6 can be removed now...

Travis CI will now use the (single) Node.js version specified in .nvmrc file. This will ensure that Node.js version used by Travis is the same as one used by developers, assuming they use nvm.

priley86 · 2018-04-12T17:26:13Z

refactor() is fine w/ me as well - but just note that it will not publish to NPM. I noted this while testing the past (neither would chore()).

#198 (comment)

Just double checking w/ you to be sure before we hit the green button.

vojtechszocs · 2018-04-12T17:34:47Z

OK, makes sense. We don't really need a new release, I'll make the first commit a refactor() then..

Use https://github.com/imsnif/synp to generate yarn.lock from package-lock.json. Fix node_modules paths. Update Travis config and project docs to use Yarn. Fix patternfly#215

priley86 · 2018-04-12T18:06:01Z

thanks @vojtechszocs 🥇 - very much appreciate this!

AllenBW reviewed Mar 27, 2018

View reviewed changes

karelhala reviewed Mar 27, 2018

View reviewed changes

AllenBW mentioned this pull request Apr 2, 2018

chore(dependencies): Update non-breaking deps #293

Merged

vojtechszocs force-pushed the migrate-from-npm-to-yarn branch from d1ab943 to 3d81218 Compare April 11, 2018 17:45

vojtechszocs force-pushed the migrate-from-npm-to-yarn branch 3 times, most recently from 39655e9 to 132c155 Compare April 12, 2018 11:02

jeff-phillips-18 previously approved these changes Apr 12, 2018

View reviewed changes

vojtechszocs added 5 commits April 12, 2018 19:37

refactor(package): Migrate from npm to Yarn (patternfly#289)

bef6c64

Use https://github.com/imsnif/synp to generate yarn.lock from package-lock.json. Fix node_modules paths. Update Travis config and project docs to use Yarn. Fix patternfly#215

Address review comments

13583a6

Update README

1fec09c

Update README on adding dependency via npm

6383495

Update npm references & improve Travis config

b11be65

vojtechszocs dismissed jeff-phillips-18’s stale review via b11be65 April 12, 2018 17:45

vojtechszocs force-pushed the migrate-from-npm-to-yarn branch from 132c155 to b11be65 Compare April 12, 2018 17:45

vojtechszocs changed the title ~~Migrate from npm to Yarn~~ refactor(package): Migrate from npm to Yarn Apr 12, 2018

priley86 approved these changes Apr 12, 2018

View reviewed changes

priley86 merged commit 332aace into patternfly:master Apr 12, 2018

josejulio mentioned this pull request May 17, 2018

KIALI-727 Migrate from npm to yarn kiali/kiali-ui#348

Merged

2 tasks

Conversation

vojtechszocs commented Mar 27, 2018

Uh oh!

vojtechszocs commented Mar 27, 2018

Uh oh!

vojtechszocs commented Mar 27, 2018

Uh oh!

AllenBW Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

karelhala Mar 27, 2018

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

karelhala commented Mar 27, 2018

Uh oh!

priley86 commented Apr 2, 2018

Uh oh!

jeff-phillips-18 commented Apr 3, 2018

Uh oh!

vojtechszocs commented Apr 5, 2018

Uh oh!

vojtechszocs commented Apr 5, 2018

Uh oh!

vojtechszocs commented Apr 5, 2018

Uh oh!

priley86 commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vojtechszocs commented Apr 9, 2018

Uh oh!

vojtechszocs commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vojtechszocs commented Apr 9, 2018

Uh oh!

priley86 commented Apr 9, 2018

Uh oh!

vojtechszocs commented Apr 11, 2018

Uh oh!

vojtechszocs commented Apr 11, 2018

Uh oh!

vojtechszocs commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1157

💛 - Coveralls

Uh oh!

vojtechszocs commented Apr 12, 2018

1, Caching

2, Node.js version

3, Pruning unused packages in node_modules

Uh oh!

priley86 commented Apr 12, 2018

Uh oh!

vojtechszocs commented Apr 12, 2018

Uh oh!

priley86 commented Apr 12, 2018

Uh oh!

vojtechszocs commented Apr 12, 2018

Uh oh!

priley86 commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

priley86 commented Apr 5, 2018 •

edited

Loading

vojtechszocs commented Apr 9, 2018 •

edited

Loading

vojtechszocs commented Apr 12, 2018 •

edited

Loading

coveralls commented Apr 12, 2018 •

edited

Loading

3, Pruning unused packages in `node_modules`