Add support for ignored config#371
Conversation
|
Hooray! All contributors have signed the CLA. |
🦋 Changeset is good to goLatest commit: 5b7b27a We got this. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for doing this! I will get to reviewing this soon. |
Andarist
left a comment
Andarist
left a comment
There was a problem hiding this comment.
The overall approach looks good and the implementation has required less code being changed than I would have anticipated. I've left some minor comments but they should be fairly easy to resolve, can't wait for tests being added and shipping this feature 🚀
| if (updatedPreState.mode === "exit") { | ||
| for (let pkg of packages.packages) { | ||
| if (preVersions.get(pkg.packageJson.name) !== -1) { | ||
| if (preVersions.get(pkg.packageJson.name)) { |
There was a problem hiding this comment.
This change is unrelated to the ignored config, but I think !== -1 is a bug which gave me a version bump to an unchanged package in my test project. @Andarist, Can you please confirm?
There was a problem hiding this comment.
I suspect this was a mistake as well - would you be able to share the repro case for the situation you have encountered? It would be great to add a test covering your fix.
There was a problem hiding this comment.
There was a problem hiding this comment.
Could you move this change to another PR? I want to have a deeper look into it and it's hard to do when combined with all these other changes.
|
Addressed all comments. I think the PR is ready to merge. Thanks @Andarist for your thorough review! |
| if (updatedPreState.mode === "exit") { | ||
| for (let pkg of packages.packages) { | ||
| if (preVersions.get(pkg.packageJson.name) !== -1) { | ||
| if (preVersions.get(pkg.packageJson.name)) { |
There was a problem hiding this comment.
Could you move this change to another PR? I want to have a deeper look into it and it's hard to do when combined with all these other changes.
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
Co-authored-by: Mitchell Hamilton <mitchell@hamil.town>
|
@Feiyang1 could you resolve conflicts? @Noviny @mitchellhamilton could you do a second round of reviews? It would be great to land this soon! |
|
@mitchellhamilton gentle ping |
|
One slight problem I have found with this feature. Upgraded dependencies of ignored packages are being written to the ignored package's This, of course, is not a big deal - I don't quite care about those dependency release lines. It's just something I've noticed now. |
5 participants
Implements #242
@Andarist Can you please take a look? Need to add tests, but want to confirm that this approach looks reasonable to folks first.