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

Upgrade eslint-plugin-react in packages #1972

Merged
merged 1 commit into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@minznerjosh
Copy link
Contributor

minznerjosh commented Jan 8, 2019

I started getting tired of seeing the red x next to my builds so I decided to figure out why the linter was failing. Lo and behold, @ljharb fixed the issue in eslint-plugin-react a few days ago!

It looks like you upgraded the dependency in the top-level package.json, but not in the child packages, so I figured I'd help you out with that chore.

P.S. I believe it is also a common monorepo pattern to only declare devDependencies in the top-level package.json. We could also move to that pattern to avoid chores like this in the future.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 8, 2019

Personally I think that's a terrible pattern; each package should be able to operate as if it's in a standalone, separate repo, as would be preferred.

@ljharb ljharb force-pushed the trialspark:josh__upgrade-eslint-plugin-react branch from 0d48478 to a325b81 Jan 8, 2019

@ljharb

ljharb approved these changes Jan 8, 2019

Copy link
Member

ljharb left a comment

For the record, the ^ means that this will be always included in ci anyways, but bumping it is good.

@ljharb ljharb added the Tests label Jan 8, 2019

@minznerjosh

This comment has been minimized.

Copy link
Contributor

minznerjosh commented Jan 8, 2019

I think it depends on what your goals are. If the goal is to make sure that the dependencies are the same across the entire project, no matter what, then it seems like a reasonable choice. You know what’s best for enzyme. 😊

And whoops. I’ve been in yarn world for the past 2 years (where versions are locked in yarn.lock) so I forgot the bump wasn’t necessary.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jan 8, 2019

Only apps should have lockfiles :-)

@ljharb ljharb merged commit a325b81 into airbnb:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment