Skip to content
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

The wrong dependents are generated in some rare cases with linked and changing to single file changesets #84

Closed
emmatown opened this issue Jun 6, 2019 · 16 comments
Assignees
Labels
enhancement New feature or request v2

Comments

@emmatown
Copy link
Member

emmatown commented Jun 6, 2019

changeset add currently doesn't care about the linked option and that creates a problem if a package is bumped at a higher level because of linked which means there should be some dependents.

A, B and C are all linked together and they are all currently at 1.0.0. B has a dependency on C.

A changeset is added:

A@major
C@patch

There will be no dependents generated. When bumping though, A and C will both be released as 2.0.0 and B won't be released which is wrong since its dependency on C is out of range now.

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

😬

I guess step 1 is having validation in bump, then making a call whether to auto-fix this vs warn and stop. Some form of check and check --fix command would help here too right?

Reason I'm thinking of this being later is you also need to cope with these two changes being between two changesets.

@emmatown
Copy link
Member Author

emmatown commented Jun 7, 2019

internal screaming because of all the times when dependents is inaccurate now

I feel like the problem here is that we're trying to make things like linked and pre-releases fit into the model of dependents being purely determined by their releases when that's not really true in many cases even beyond those two features. Here's another example:

I have one package A@1.0.0 in a monorepo. I add a changeset with a major release.

I add a new package B@1.0.0 which has a dependency on A. I add a changeset with a major release on B. Will the dependency on B be updated since the changeset for A doesn't include the dependent on B?

A check and check --fix command would solve this particular case but I feel like having changesets become invalid and need fixing over time is a bad thing and a --fix command wouldn't fix the case where changes are between two changesets because where would the dependents go since the dependents would be determined by multiple changesets?

I feel like the answer to all of this is to remove the dependents field from a changeset. I don't totally know about this and of course it means that a person won't know all the packages that will be released when creating a changeset but as we know, that's sometimes wrong and IMO not knowing what will be released > being told incorrect information about what will be released.

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

not knowing what will be released > being told incorrect information about what will be released

Probably true. I'd want to modify the status command so it would tell you extra stuff it was going to release, but otherwise fine.

I think I want to do this as a breaking change?

This leaves us with two breakin changes to do:

  1. remove dependents from changeset object - generate dependents to be bumped as part of bump, not add.
  2. Redo the changesets config

Does this sound right?

@emmatown
Copy link
Member Author

emmatown commented Jun 7, 2019

Maybe if we’re going to remove the dependents, we could consider the single file changeset thing again since the thing that was potentially bad to modify is gone? It could be the same syntax as the add a changeset magic as a markdown file. People could even write changesets manually if they wanted to. If we did that, a check command would probably be quite important to ensure that changsets only contain packages that exist in case of typos and etc.

Those breaking changes sound good to me(though I still have config thoughts which I’ll add to that issue soon).

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

we could consider the single file changeset thing

Strong agree

It could be the same syntax as the add a changeset

Also strong agree - the syntax is notably more human-friendly that making frontmatter fields

a check command would probably be quite important

Technically status will need to perform a check, and you can make it be the thing that causes failure, but probably cleaner as its own command.

This set of breaking changes is probably a good place to do proper thoughtful package splitting. I'm guessing most of the commands will get their own package (without the questions) so anyone can programmatically do any of our steps. This will probably involve making our own implementation of release (we pretty much still just run yarn's command).

@emmatown
Copy link
Member Author

emmatown commented Jun 7, 2019

This will probably involve making our own implementation of release (we pretty much still just run yarn's command).

Nit: you mean npm not yarn for publishing right?

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

I actually meant bolt... buy yes, npm would have been more correct as well.

@emmatown
Copy link
Member Author

emmatown commented Jun 7, 2019

Also strong agree - the syntax is notably more human-friendly that making frontmatter fields

I wrote a changeset like this and ran prettier on it.

- react-select@major
---

My summary here.

Then it turned into

- react-select@major

---

My summary here.

I feel like the spacing is not ideal so maybe we should make it YAML frontmatter like this?

(I don't totally understand how this would be less human readable or were you thinking of a different way of expressing this in front matter?)

---
react-select: major
---

My summary here.

It also displays really nicely in GH https://github.com/mitchellhamilton/changeset-test

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

Ah cool. This is nice now because you don’t have to nest the list of packages (having to have dependents as an array and releases as an array got messy)

Once it’s pkgname: type it’s very readable.

@Noviny
Copy link
Collaborator

Noviny commented Jun 7, 2019

Aside on moving formats, we can actually make it a non-breaking change by supporting bumping off both changeset types for a while, then make a breaking change change when we drop the old one. I am keen to get this in ASAP tbqh, as well as start finding the edge cases.

Guess last question is who should take a stab at implementing this/is there any pre-work we want to invest in?

@emmatown
Copy link
Member Author

emmatown commented Jun 7, 2019

Strong agree on adding it before a major.

Do you wanna try implementing it?

I was going to say that maybe I should seperate out the create changeset package in the GitHub comment PR to another PR that can be merged but practically all of the logic is about calculating dependents so it’s probably better to start from the current state rather than that. I’ll seperate some TS conversion things from that into another PR though.

@emmatown emmatown changed the title The wrong dependents are generated in some rare cases with linked The wrong dependents are generated in some rare cases with linked and changing to single file changesets Jun 7, 2019
@Noviny Noviny self-assigned this Jun 11, 2019
@Noviny Noviny added the enhancement New feature or request label Jun 11, 2019
@Noviny Noviny mentioned this issue Jun 11, 2019
@Noviny
Copy link
Collaborator

Noviny commented Jun 11, 2019

we can actually make it a non-breaking change

Scratching this - it will cause the packages that are released to be different, and we shouldn't change that logic without breaking. Still working on this though.

@Noviny
Copy link
Collaborator

Noviny commented Jun 13, 2019

SO documenting a discussion on the parts and packages we need to do this, as well as make it easy for other people to build upon.

@changesets/parse -> takes in the new format for a changeset and converts it to JSON { summary, releases }
@changesets/read -> reads in the changesets from fs, and outputs them as [{ summary, releases }] (using parse)
get-dependents-graph package from bolt extracted into own package
@changesets/determine-dependents -> Takes in output of read, and other functions to determine dependents - doesn't itself touch fs
@changesets/get-release-info -> composes read and determine-dependents to output { changesets, dependents } as an object for status or bump to rely on as output.

@emmatown
Copy link
Member Author

{ summary, releases }

Maybe this should include the name of the changeset(as in the random thing)? Though then I guess that wouldn't make sense for the add a changeset bot thing though i suppose the bot could just not parse it. The reason I'm thinking about this is #90, changelog entry generators need some way to get the commit and you need some reference to a file system thing to do that so we need a way to store that in our internal representation of a changeset.

@Noviny
Copy link
Collaborator

Noviny commented Jun 13, 2019

Perhaps { summary, releases, ?id }?

I think parse ignores id, but read can auto-provide id.

@emmatown
Copy link
Member Author

Yeah I guess that would work. There's something that slightly worries me about having an optional thing like that because I'm afraid we or others will accidentally assume that it always exists. (e.g. commit can technically be undefined right now but all the changelog entry generators i've seen/written assume it always exists)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2
Projects
None yet
Development

No branches or pull requests

2 participants