defaultWrittenConfig change baseBranch from master to main #677
Conversation
|
Hooray! All contributors have signed the CLA. |
|
packages/config/src/index.test.ts
Outdated
| @@ -40,7 +40,7 @@ test("read reads the config", async () => { | |||
| changelog: false, | |||
| commit: true, | |||
| access: "restricted", | |||
| baseBranch: "master", | |||
| baseBranch: "main", | |||
There was a problem hiding this comment.
Correct me if I'm wrong but it looks like this is actually a breaking change. I've looked into the new-config fixture and it only has such .changeset/config.json:
{
"changelog": false,
"commit": true
}I think the point of this PR was only to initialize the .changeset/config.json with the new value but not to change what it defaults to when it is not explicitly given etc.
There was a problem hiding this comment.
I think the point of this PR was only to initialize the .changeset/config.json with the new value but not to change what it defaults to when it is not explicitly given etc.
What's the difference there?
There was a problem hiding this comment.
@Andarist Happy to make changes here as necessary but I didn't quite understand your point.
There was a problem hiding this comment.
The problem with the current solution is that if somebody already has a config like:
{
"changelog": false,
"commit": true
}then with this change Changesets would start using main as their baseBranch and thus it would change behavior, by potentially crashing because a main branch might not even exist in a given repository.
When initializing Changesets we write defaultBranch into the generated .changeset/config.json, even though it's not strictly required because that property has a default. But since we are outputting this property it can be changed somewhat safely because people who are already using Changesets are not running changesets init, they have already done that in the past.
There was a problem hiding this comment.
I see, makes sense. So we need two separate objects, one like defaultWrittenConfig which sets the default values in a new config file and another defaultConfigValues that sets the values for configs that don't specify optional keys??
There was a problem hiding this comment.
Ye, smth like that would work. Please only that because this is a monorepo we might have to implement this in a non-intuitive way. We don't want to break people who don't upgrade all of the transitive dependencies at once.
I think that the safest solution is to not touch defautlWrittenConfig at all (as weirdly as it might be) - and to just override this value before we actually write it in the config file.
There was a problem hiding this comment.
I think that the safest solution is to not touch defautlWrittenConfig at all (as weirdly as it might be) - and to just override this value before we actually write it in the config file.
I agree, seems like the best solution here.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit b397c6a:
|
|
@Andarist Could you let me know if this change is still acceptable? |
| @@ -44,7 +46,7 @@ export default async function init(cwd: string) { | |||
| await fs.copy(path.resolve(pkgPath, "./default-files"), changesetBase); | |||
| await fs.writeFile( | |||
| path.resolve(changesetBase, "config.json"), | |||
| JSON.stringify(defaultWrittenConfig, null, 2) | |||
| JSON.stringify({ ...defaultWrittenConfig, baseBranch: "main" }, null, 2) | |||
There was a problem hiding this comment.
I would extract this to a variable on the top of the file, so the added comment could be added to that variable so it would be visible~ in a common place for both places that have been touched here.
|
@janosh sorry for the delay, it's just sometimes hard to get to everything on time. The PR looks good - I just left a single nit-level comment. Could you also add a changeset to this PR? |
Closes #675.
Should I add a changelog entry?