-
Notifications
You must be signed in to change notification settings - Fork 507
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
Add new config option for always upgrading internal dependents #542
Conversation
🦋 Changeset detectedLatest commit: 8337e00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this as an experimental option to start?
This was my initial intention but then I've come to the conclusion that this is what has caused multiple problems already - both in our repositories and for users. While we probably shouldn't change to this strategy as a default I suspect that we'd like to do it in the next major version. Besides bikeshedding on how this should be called and under what option this should live - I don't really think if we have any doubts that this has to be introduced in some way or another. I view the experimental options as stuff that we are unsure about just yet. |
Ok, can you change the option to something like this that makes it clear what the different states do and add some documentation? updateInternalDependents: "always" | "out-of-range" |
Sure, the proposed values make much more sense than a simple boolean. Are you OK with the chosen name for the option though? Or do you maybe have any better alternatives for it? Also I would appreciate your thoughts regarding this:
|
I don't think this is true, updateInternalDependencies would still work. updateInternalDependencies is extremely confusing though. Yeah, this is why I wanted to have it as an experimental option so this can be bikeshed without blocking the usage of this. So could we put this into an experimental option so that we can spend more time thinking about all the config together and probably doing a major then? |
21ca841
to
674ed0f
Compare
In what situation this would be relevant? I guess it would be possible to add a dependent to the release (patch bump) but skip updating a dependency range if the I'm not sure though if we should care that much about this as people are already confused quite a bit when it comes to
Done that just now, please take a look :)
I like the idea of making a major version some time soon-ish but also our main focus is not on the project so I wonder when this has a chance to happen and what kind of things we'd like to include in that major. Including this as a stable option right away was also somewhat backed by this situation - it was, in my eyes, an opportunity to bikeshed this as I was worried if I would start with the experimental option then we would just merge this in and forget about refining this further. However, I'm OK with making this an experimental option right now, since I need this to land much more than I need to bikeshed over this 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs changesets(and they should be major for types and anything that accepts a parsed config as an argument)
packages/config/src/index.ts
Outdated
@@ -16,6 +16,7 @@ export let defaultWrittenConfig = { | |||
access: "restricted", | |||
baseBranch: "master", | |||
updateInternalDependencies: "patch", | |||
updateInternalDependents: "out-of-range", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be here, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, forgot about reverting this one as TS didn't complain about it.
@@ -73,7 +73,7 @@ function assembleReleasePlan( | |||
changesets: NewChangeset[], | |||
packages: Packages, | |||
config: Config, | |||
preState: PreState | undefined, | |||
preState?: PreState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert this? this was intentionally required to prevent accidentally not reading and passing preState.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm reverting this and adding a comment about this
Yeah, that. I'm not saying that it necessarily be useful or behavior that you would want just that it has coherent behavior that is separate from updateInternalDependents.
Yeah, I agree that it's not ideal, tbh though I'd rather that we forget about refining this further than hastily include a solution that we later come to regret. |
.changeset/light-buttons-chew.md
Outdated
@@ -0,0 +1,17 @@ | |||
--- | |||
"@changesets/cli": minor | |||
"@changesets/config": major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a major for @changesets/config
since @changests/config doesn't accept a parsed config, it returns one, it's a major for assemble-release-plan and etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option kinda saved us from needing to patch-package changesets, as we always publish to a CDN with fixed versions, it's important for us to be able to always update dependents when a dependency inside the monorepo is bumped. I think that use case is perhaps what you guys are looking for to make this a stable instead of experimental option? |
Why isn't there an option to run off this behavior if desired? If pkg-a depends on pkg-b and pkg-b is bumped and for whatever reason, it breaks pkg-a (not saying in an ideal world this should happen, but we live in the real world and this does happen). Isn't there an option to disable the bumping...? |
I'm sorry for some unrelated changes to naming, internal function parameters, etc but they were bugging me or things were slightly harder to read at times to build a proper mental model about the codebase. So I've just adjusted those in the process.
The chosen name for the option can be easily mistaken with already quite confusing
updateInternalDependencies
so I'm very open to changing that - maybe smth like:bumpInternalDependents
?upgradeInternalDependents
?alwaysBumpInternalDependents
?In fact - using
updateInternalDependencies
together withupdateInternalDependencies
doesn't make sense. The new option (if used) should always take precedence. So maybe it would be worthwhile to deprecate theupdateInternalDependencies
and group both under smth likeinternalBumpingStrategy
?