v6: lerna, deps, react fix #75
Conversation
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "lerna": "2.5.1", | |||
| "version": "6.0.0", | |||
There was a problem hiding this comment.
could use independent versioning, but not biggie
There was a problem hiding this comment.
Was intentional to not use independent. 😄
If you have some good reading on why independent is preferable, I'm happy to read and reevaluate on the next release.
There was a problem hiding this comment.
Short of it is that even though you make a breaking change in the react config, there's no need to make a breaking change in all of the other packages
There was a problem hiding this comment.
Understood. That was a consideration when choosing but my preference is fixed versions. Inspecting a package.json it's easy to tell when you have compatible versions. This isn't an issue if you're on the latest versions, but if you're a few behind you have more effort to look up which package goes with which.
|
@SimenB I've updated the dependencies as we discussed, but I've removed all peerDependencies in favour of pure dependency usage. |
package.json
Outdated
| @@ -25,7 +21,8 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "eslint-config-schibsted": "file:./packages/eslint-config-schibsted", | |||
| "eslint-config-schibsted-node": "file:./packages/eslint-config-schibsted-node" | |||
| "eslint-config-schibsted-node": "file:./packages/eslint-config-schibsted-node", | |||
There was a problem hiding this comment.
you shouldn't need these as yarn hoists them up automatically
There was a problem hiding this comment.
Ah yes, with the workspaces. Patch incoming.
| @@ -12,15 +12,15 @@ acorn@^3.0.4: | |||
| version "3.3.0" | |||
There was a problem hiding this comment.
delete nested yarn lockfiles
There was a problem hiding this comment.
I would have expected that to be cleaned up automatically.
Done.
| }, | ||
| "peerDependencies": { | ||
| "eslint": "^4.10.0" | ||
| "eslint": "^4.13.1", |
There was a problem hiding this comment.
I wonder if eslint itself should be a peer, but not the configs and plugins?
There was a problem hiding this comment.
It would gain is a warning when installing, but I don't think it improves the resolution process. As we discussed originally my plan was to leave eslint as the only peer, but then I thought maybe it didn't deserve to be special cased since any package version conflicts will give unpredictable outcomes.
This method also means that eslint doesn't have to be specified at the root. How likely that is I'm unsure since I don't know how others will mix this with other linting packages.
When you go to update the proxy config for Finn, will the peer warning for just eslint be sufficiently beneficial?
There was a problem hiding this comment.
This is about allowing consumers to control the version of eslint independently from the configs
any package version conflicts will give unpredictable outcomes.
how so?
When you go to update the proxy config for Finn, will the peer warning for just eslint be sufficiently beneficial?
yes
There was a problem hiding this comment.
Correct my understanding where it's wrong.
Consumers can control the version of any package by using the resolutions section of package.json.
If a eslint config/plugin has multiple versions installed, the version at the root will be the version used when eslint executes. The version in the root might not be the version that we wanted. I believe this would be the reasoning for AirBnB to make all eslint plugins/configs peerDependencies.
You said yes to the warning about eslint, and only eslint, would provide benefit. Given the above, can you please clarify why? I would have assumed that warnings for all or warnings for none would be the preferable approach.
I could go either way.
I originally had a lot more in here about how peerDependency installs could go wrong, but that just lead me to thinking that specifying peerDependencies is the better option. Packages like ESLint that require a CLI to read from the root node_modules seem like the poster package for peerDependencies. It doesn't stop version conflicts, but it does expose the warnings.
But...
The warnings shortcut debugging, but I'm getting more blind to them with the recent bug(s) with false positives. And there will be situations where not all of you dependencies have updated to the latest eslint/config/plugin. So you'll either have to choose to not update or see the error and get unexpected outcomes anyway.
Plus not having to install things in the root that I'm not actually using myself is nice.
In the case of eslint-config-finn, you're going to have a dependency that is 3 levels removed from the root package.json. Do we really want a peerDependency chain so far removed from its use?
eslint-config-finn -> eslint-config-schibsted -> eslint-config-airbnb -> eslint-plugin-import
These will need to be installed at the root, with some appearing unused:
eslint
eslint-plugin-import
eslint-plugin-jsx-a11y
eslint-plugin-react
eslint-plugin-chai-friendly
eslint-plugin-flowtype
eslint-config-airbnb-base
eslint-config-airbnb
eslint-config-prettier
eslint-plugin-prettier
eslint-plugin-unicorn
prettier
... and then...
I've just spotted some >= versioning, so even if we get rid of all the unmet peerDependency messages, we might still be getting unexpected behavior because of changes in major versions.
After all that I'm actually leaning towards making them all peerDependencies. Hopefully I've gotten something wrong and you can talk me out of it. 😁
There was a problem hiding this comment.
Here's an example of how having it as dependencies is a potential footgun, and why peer deps might be preferred: eslint/eslint#8547
Because of semver, the only way to ensure a single version is really to use peer deps, but it's annoying. See eslint/eslint#3458
I think maybe plugins should be peer deps, while config can be deps. (in practice)
Eslint itself should be peer. And prettier should be peer.
Agreed?
There was a problem hiding this comment.
Thanks for doing the leg work finding those threads. It's a shame people discovered these issues 2 years ago and it still isn't resolved.
After reading those threads and a lot of tangents is confirmed my assertions above that the benefit of using peerDependencies is the warning message on install. It doesn't prevent version conflicts.
Anyway, I've updated the code to use peerDependencies, and corrected the use of configs so that they are at least the correct version. This can't be done nicely for configs that come from plugins, so I have just given in that there may be plugin/config mismatches as well as plugin mismatches.
There was at least one extra peerDependency that was required, but want listed as a peer in the dependency. Hopefully I've covered the lot.
READMEs updated to reflect the requirements.
|
Please give another review with the latest changes. I'll squash on merge. |
| @@ -4,5 +4,5 @@ module.exports = { | |||
| env: { | |||
| node: true | |||
| }, | |||
| extends: ["./chai.js"].map(require.resolve) | |||
There was a problem hiding this comment.
then eslint will just see ./chai.js and will have to figure out the path. simpler and clearer to give absolute path IMO
There was a problem hiding this comment.
The main benefit of using the require.resolve is that it forces resolution to the packages specified in this packages package.json instead of the package that eslint finds in the root node_modules. This isn't a concern for the relative path since it's relative to this package. (Although this wasn't one of the tests I did.)
I also made the call that having less code here was better. I don't know the speed differences of resolving the path here, versus it being resolved somewhere later, but I'm presume it's negligible in this use case.
Happy to revisit, but will merge as-is since I have your approval.
Not separate commits, sorry.
Fixed eslint-config-schibsted-react because I borked the SPT merge and missed a dependency.
Update some dependencies including unicorn major bump to V3.
Add Lerna to resolve #44