Skip to content

Commit

Permalink
You need a yarn.lock for Travis to build in yarn mode.
Browse files Browse the repository at this point in the history 
This should be included so that installations are consistent and we're all building/testing/developing against the same deps.
  • Loading branch information
@timdorr
timdorr committed Mar 28, 2017
1 parent 9b5d053 commit 2aaab3c
Show file tree
Hide file tree
Showing 2 changed files with 589 additions and 1 deletion.
1 change: 0 additions & 1 deletion .gitignore
View file
Expand Up @@ -3,4 +3,3 @@ lerna-debug.log
npm-debug.log
yarn-error.log
packages/*/yarn.lock
yarn.lock

7 comments on commit 2aaab3c

@mjackson
Copy link
Member

Choose a reason for hiding this comment

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

OK, so we need a yarn.lock in the root but not in the individual packages directories? I don't fully understand why, but it seems like that's what React core does so if it's good enough for them...

@timdorr
Copy link
Member Author

Choose a reason for hiding this comment

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

Travis looks for it and switches all the npm commands for yarn commands. That's the main reason here.

Lerna's support for yarn is new, so things like upgrading all the packages' yarn.lock files together aren't quite yet there. But it speeds up the build enough for the thing not to time out (we're teetering on the edge as-is...), so it's necessary.

@mjackson
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @timdorr. The monorepo really slowed things down. I've been investigating ways to try and speed things up, but I couldn't get lerna's --hoist functionality to work for us yet. Gonna keep trying.

@pshrmn
Copy link
Contributor

@pshrmn pshrmn commented on 2aaab3c Mar 30, 2017

Choose a reason for hiding this comment

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

Have you considered limiting the browser tests to react-router-dom? Or possibly just rely on history doing cross-browser checks? The only time that I recall those catching anything has been with mobile safari not supporting Object.assign. Its obviously nice to catch those sort of things, but it seems like overkill to have to check every browser for every commit.

It would be nice if there was a babel plugin that lets you specify browser versions to target and yells at you if you use something unsupported. I don't have the mental model of ASTs yet to know if that is possible, but I feel like it should be.

@timdorr
Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't that just babel-preset-env?

@pshrmn
Copy link
Contributor

@pshrmn pshrmn commented on 2aaab3c Mar 30, 2017

Choose a reason for hiding this comment

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

I was thinking more along the lines of a warning instead of modifying the build, which appears to be doable with eslint-plugin-compat.

Maybe it would make sense to just have babel polyfill automatically, but I'm not sure if that would bloat the build.

Looking into this, I also saw that there are a couple existing linting errors. Guess no one has been using eslint 😛

@mjackson
Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea, @pshrmn. Running everything every time on every browser does seem like overkill.

Please sign in to comment.