-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use Yarn for dependencies on Circle CI #3364
Conversation
👍 I definitely dig this idea, and I agree re: waiting for a bit more community vetting before making the switch. Barring any post-release issues cropping up, I don't see any downsides. |
👍 on this.
Worth noting that this deviates from the usual practice for package managers in the style of Yarn, Bundler, and Cargo, which is to check in lockfile for apps but not for libs (since the lockfile doesn't actually affect what gets installed via |
yarn lists the lock file as required under their notes on version control. https://yarnpkg.com/en/docs/version-control#toc-required-files |
I think it's probably an oversight or incomplete documentation. I'd be surprised if the actual best practice settles on something different than what http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/ and http://doc.crates.io/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries recommend. |
I just posted an issue to yarn to see if they have more details on why they check in the lock file. |
Need to check that checking in yarn.lock doesn't cause an issue like #3374. |
https://twitter.com/thejameskyle/status/787077917331320832 indicates we'd be okay in that regard. |
Since yarn doesn't respect dependency |
@mcwhittemore this is only relevant for people who use GL JS with Browserify. The other 99.9% will use the released built version, and we want to make sure that it's the same as the one we have locally, on Circle and on master, otherwise bugs in downstream modules will slip by and we won't have a good way to catch them until someone reported an error. |
I just hit a bug with yarn + git dependencies: yarnpkg/yarn#1171. Seeing quite a few similar ones: yarnpkg/yarn#1087, yarnpkg/yarn#1280, yarnpkg/yarn#1278, yarnpkg/yarn#1017. Since we depend on this usage pattern for mapbox-gl-function, mapbox-gl-shaders, mapbox-gl-style-spec, and mapbox-gl-test-suite, we'll need to wait for these sorts of bugs to be fixed. |
The yarn bugs with git dependencies are not fixed as of yarn 0.17.3. |
@jfirebaugh does it have the same buggy behavior even if you clean cache / remove yarn.lock once? They now use tarball hash in cache names. |
Yes, I cleaned everything before testing. |
OK, this seems to be ready to merge. Note that to make CI run Yarn for style spec as well, I have to run the root dir |
Also note that currently the "latest" Yarn installed on CI is still 0.19.1 (without the git hash fix) — 0.20.0 is considered RC per yarnpkg/yarn#2616, but it should be not matter in this case because we don't have any sha deps left, and the problem will solve itself in a few days. |
🎉 !
Seems fine to me, but maybe worth sticking a comment in the |
I'm quite puzzled by the recent failures with |
2aa2d2b
to
8f48c28
Compare
Tried Node 6 locally and the tests pass. Rebased on master, regenerated Yarn.lock, ran the build without cache and it still fails on Circle. Will have to SSH into the build to investigate further... |
Finally figured out what was wrong. The Found a more elegant solution to the |
Rebased, merging on green. |
This is just a proof of concept for using Yarn on Circle CI and having a Yarn lockfile for local development. You can see this sample build taking the same time as our NPM3 one (~7 min). Non-cached build currently takes around ~5-6 min longer, but most of it is because of prebuild/prebuild#132 (headless-gl recompiling every time).
I'm not suggesting to switch to Yarn now, but want to have a discussion about a potential switch in future, after it slightly matures following the public release.
Benefits of using Yarn with a lockfile in the repo instead of NPM:
rm -rf node_modules && npm cache clean && npm install
— no longer!npm update
on master builds if we have locked versions everywhere. Updating to the latest versions can be explicitly done withyarn upgrade
locally (fast) and then committingyarn.lock
changes.Note that I removed caching
~/.yarn-cache
between builds because it adds a few minutes to the build, and is not necessary in this case becausenode_modules
are cached anyway, and we only runyarn
once.@jfirebaugh @lucaswoj @mollymerp @anandthakker