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

Switch to Yarn #15485

Merged
merged 26 commits into from Jan 10, 2018

Conversation

Projects
None yet
3 participants
@spalger
Member

spalger commented Dec 7, 2017

Resubmission of #8621

Closes #8621

This PR includes a vendored version of Yarn built from yarnpkg/yarn#5059.

@spalger spalger added the WIP label Dec 7, 2017

"mathjs": "^3.16.2",
"minimatch": "^2.0.10",
"mkdirp": "^0.5.1",
"moment": "^2.13.0",

This comment has been minimized.

@spalger

spalger Dec 8, 2017

Member

By loosening our required versions (in package.json, yarn.lock makes them strict) we allow yarn to flatten our node_modules more. This is good for efficiency, but also necessary for some of our dependencies

With strict versions in Kibana yarn was installing 12 copies of moment.

# find node_modules -path '*node_modules/moment'
node_modules/moment-timezone/node_modules/moment
node_modules/statehood/node_modules/moment
node_modules/moment
node_modules/hapi/node_modules/moment
node_modules/libesvm/node_modules/moment
node_modules/grunt-esvm/node_modules/moment
node_modules/inert/node_modules/moment
node_modules/bunyan/node_modules/moment
node_modules/vision/node_modules/moment
node_modules/h2o2/node_modules/moment
node_modules/even-better/node_modules/moment
node_modules/makelogs/node_modules/moment

This caused bugs and test failures because we rely on moment having the same internal state everywhere, especially in moment-timezone, but moment-timezone depends on moment@^2.6.0. With npm this a satisfied by our dependency on moment@2.13.0, but yarn attempts to get the latest version of moment it can for moment-timezone, causing it to install moment@2.19.3 into moment-timezone/node_modules. When Kibana and moment-timezone depend on a ^ version of moment they both get the most recent version, installed once in the root node_modules directory.

With looser versions, yarn installs one version of moment

node_modules/moment

Overall, with strict versions kibana installs 2521 modules (88k files) and with looser versions kibana installs 2296 modules (82k files). A decent win.

@spalger spalger added the blocked label Dec 8, 2017

@kimjoar

This comment has been minimized.

Contributor

kimjoar commented Dec 9, 2017

I've got a PR in to fix the blocker: yarnpkg/yarn#5059. Let's see if it's accepted.

@kimjoar kimjoar changed the title from switch to yarn to Switch to yarn Dec 9, 2017

@kimjoar kimjoar changed the title from Switch to yarn to Switch to Yarn Dec 9, 2017

@tylersmalley tylersmalley self-requested a review Dec 13, 2017

@tylersmalley tylersmalley removed their request for review Dec 26, 2017

@tylersmalley

This comment has been minimized.

Member

tylersmalley commented Dec 26, 2017

@spalger, un-assigning myself until the yarn PR is merged.

@spalger

This comment has been minimized.

Member

spalger commented Jan 8, 2018

NOTE: need to verify that grunt licenses:csv_report produces the same output with yarn and npm

// We rely on a local version of Yarn that contains the bugfix from
// https://github.com/yarnpkg/yarn/pull/5059. Once this fix is merged
// and released we can use Yarn directly in the build.
const yarn = `${__dirname}/../vendor/yarn-1.3.2-with-ignore-fix.js`;

This comment has been minimized.

@spalger

spalger Jan 9, 2018

Member

I think require.resolve('../vendor/yarn-1.3.2-with-ignore-fix.js') is a better option than string concatenation.

This comment has been minimized.

@kimjoar

kimjoar Jan 9, 2018

Contributor

Most definitely! Fixed

@@ -3,6 +3,7 @@ module.exports = function () {
devSource: {
options: { mode: true },
src: [
'yarn.lock',

This comment has been minimized.

@spalger

spalger Jan 9, 2018

Member

Makes sense to add yarn.lock to the build, but we should also remove it after installing dependencies.

This comment has been minimized.

@kimjoar

kimjoar Jan 9, 2018

Contributor

++ done

kimjoar added some commits Jan 9, 2018

@kimjoar

This comment has been minimized.

Contributor

kimjoar commented Jan 9, 2018

I made some review fixes, plus I locked some versions specifically in the yarn.lock file so we don't have several versions of angular, moment, etc

@kimjoar

This comment has been minimized.

Contributor

kimjoar commented Jan 9, 2018

Discussed with @spalger, went with resolutions instead, to make sure we force one specific version of Angular, React, Moment.

e.g. because of the outdated React packages right now we get this warning because of forcing this:

warning Resolution field "react@16.0.0" is incompatible with requested version "react@^15.6.2"
@kimjoar

This comment has been minimized.

Contributor

kimjoar commented Jan 10, 2018

jenkins, test this

The currently vendored version of Yarn contains the bugfix in
https://github.com/yarnpkg/yarn/pull/5059, which handles optional dependencies.
To build a new Yarn bundle, check out that pull requst and run:

This comment has been minimized.

@tylersmalley
To build a new Yarn bundle, check out that pull requst and run:
```
yarn run build-bundle

This comment has been minimized.

@tylersmalley

tylersmalley Jan 10, 2018

Member

Can omit run

###
### downloading yarn
###
yarnVersion="$(node -e "console.log(String(require('./package.json').engines.yarn || '').replace(/^[^\d]+/,''))")"

This comment has been minimized.

@tylersmalley

tylersmalley Jan 10, 2018

Member

Shouldn't we be using tasks/vendor/yarn-1.3.2-with-ignore-fix.js or am I missing something?

This comment has been minimized.

@kimjoar

kimjoar Jan 10, 2018

Contributor

Yeah, we should probably do that. I'll fix.

This comment has been minimized.

@spalger

spalger Jan 10, 2018

Member

We're going to need to put this back once yarnpkg/yarn#5059 is merged but 🤷‍♂️

@tylersmalley

This comment has been minimized.

Member

tylersmalley commented Jan 10, 2018

Some more pros, like we need more...

  • bundle size reduction of 6.8% (74913714 vs 69813592 bytes)
  • file count reduction of 7.47% (54408 vs 50344)

kimjoar added some commits Jan 10, 2018

@spalger

LGTM

@kimjoar kimjoar removed the v5.6.6 label Jan 10, 2018

@kimjoar kimjoar merged commit 0fde087 into elastic:master Jan 10, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

kimjoar added a commit to kimjoar/kibana that referenced this pull request Jan 10, 2018

Switch to Yarn (elastic#15485)
* switch to yarn

* cleanup misc references to npm

* [yarn] loosen dependency ranges so yarn will merge more deps

* fix linting error now that moment uses ESM

* [licenses] font-awesome changed the format of its license id

* Use local yarn

* Misc fixes

* eslintignore built yarn file

* Remove mkdir which doesn't do what it should do

* Check build without upgrading lots of versions

* Fix license check

* too many moments

* Better description

* Review fixes

* Lock to angular@1.6.5

* More specific version locks

* Revert "More specific version locks"

This reverts commit 11ef811.

* Revert "Lock to angular@1.6.5"

This reverts commit 3ade68c.

* rm yarn.lock; yarn

* Forcing a specific version of React, Angular, Moment

* Using vendored version of yarn in ci

* Use --frozen-lockfile

* fixes

kimjoar added a commit to kimjoar/kibana that referenced this pull request Jan 10, 2018

Switch to Yarn (elastic#15485)
* switch to yarn

* cleanup misc references to npm

* [yarn] loosen dependency ranges so yarn will merge more deps

* fix linting error now that moment uses ESM

* [licenses] font-awesome changed the format of its license id

* Use local yarn

* Misc fixes

* eslintignore built yarn file

* Remove mkdir which doesn't do what it should do

* Check build without upgrading lots of versions

* Fix license check

* too many moments

* Better description

* Review fixes

* Lock to angular@1.6.5

* More specific version locks

* Revert "More specific version locks"

This reverts commit 11ef811.

* Revert "Lock to angular@1.6.5"

This reverts commit 3ade68c.

* rm yarn.lock; yarn

* Forcing a specific version of React, Angular, Moment

* Using vendored version of yarn in ci

* Use --frozen-lockfile

* fixes

kimjoar added a commit that referenced this pull request Jan 10, 2018

[6.x] Switch to Yarn (#15485) (#15955)
* Switch to Yarn (#15485)

* switch to yarn

* cleanup misc references to npm

* [yarn] loosen dependency ranges so yarn will merge more deps

* fix linting error now that moment uses ESM

* [licenses] font-awesome changed the format of its license id

* Use local yarn

* Misc fixes

* eslintignore built yarn file

* Remove mkdir which doesn't do what it should do

* Check build without upgrading lots of versions

* Fix license check

* too many moments

* Better description

* Review fixes

* Lock to angular@1.6.5

* More specific version locks

* Revert "More specific version locks"

This reverts commit 11ef811.

* Revert "Lock to angular@1.6.5"

This reverts commit 3ade68c.

* rm yarn.lock; yarn

* Forcing a specific version of React, Angular, Moment

* Using vendored version of yarn in ci

* Use --frozen-lockfile

* fixes

* Update lockfile

@spalger spalger deleted the spalger:implement/yarn branch Jan 10, 2018

@spalger

This comment has been minimized.

Member

spalger commented Jan 10, 2018

6.2/6.x: a817517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment