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

Prereleases #183

Merged
merged 50 commits into from
Oct 31, 2019
Merged

Prereleases #183

merged 50 commits into from
Oct 31, 2019

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Oct 9, 2019

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2019

🦋 Changeset is good to go

Latest commit: e14c733

We got this.

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

yarn changeset prerelease next
```

This command changes Changesets into prerelease mode which creates a `pre.json` file in the `.changeset` directory which stores Some Information Which I Haven't Totally Figured Out Yet.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back to pre.json over putting it in the config because I thought having it another file would make it less likely for people to manually edit it.

… probably, I want to have an implement that works and is what we want first and things in the right places second)
@Noviny
Copy link
Collaborator

Noviny commented Oct 9, 2019

Arghfldfdlssjbwjxyil damn GitHub on mobile. Anyway.

I fundamentally agree with the approach but want to tweak some of the user commands. The singular goal is making is impossible to do things ‘accidentally’ as opposed to being fault-tolerant.

Move exit command to:

yarn changesets prerelease —exit

This makes it a binary switch. Running prerelease [TAG] while in a prerelease should error, trying to exit should error in a non-pre mode.

The related one is I want to not have it be run yarn changesets version to do the versioning. This should throw an error pointing out the prerelease state and pointing to the correct command.

My weakest point is I don’t have a command to use here. Some options:

  • changesets prerelease —version
  • changesets version —prerelease
  • something else

Basically I want to make it a unique command to switch in, to version in, and to exit.

Interestingly I don’t care about publish, but I am a bit suspicious that it’s just because I know what publish does and how it won’t really care about this internally so I don’t externally care about it appearing different. I think it’s fine though.

There’s an argument that this kind of tagging can be done for things other than pre-releases that I am having with myself but I think I don’t care. Anyone doing such custom publishes I just trust to ignore such words.

Implementation detail, but I would love if publish in a pre-state corrected for and avoided overwriting ‘latest’ on npm to make sure those installs happen fine. This may be related to hotfixing patches though and pre-releases might just be fine.

Docs notes for eventually:

  • When to use pre
  • complexities of pre with monorepos
  • complexities of two publish branches in a monorepo and ways it will make you cry.

I may add more later, but overall these are really nits. Most important take aways are I agree with the direction and ideas of this and am super keen to see it fleshed out and implemented 🦋❤️

@emmatown
Copy link
Member Author

emmatown commented Oct 9, 2019

Basically I want to make it a unique command to switch in, to version in, and to exit.

The reason I wanted to not do this is so that we can leverage existing release infrastrucure(i.e. the release action) so essentially we can think of the prerelease state as a really special changeset but I'm interested to hear why a separate command might be better?

I assume this is because mobile keyboards and you mean --?

Implementation detail, but I would love if publish in a pre-state corrected for and avoided overwriting ‘latest’ on npm to make sure those installs happen fine. This may be related to hotfixing patches though and pre-releases might just be fine.

Are you referring to the new package during a pre state case writing to latest because otherwise publish should make the dist tag to the tag you specify so it won't overwrite latest.

@Noviny
Copy link
Collaborator

Noviny commented Oct 10, 2019

I assume this is because mobile keyboards and you mean --?

Yes this

publish should make the dist tag to the tag you specify

This - looks like it should be fine.

why a separate command might be better

My reasoning is basically pre-releases are a super-special space. Once you switch into them, things behave differently, so I want to make taking actions different too.

The main case is so you can never take actions unaware that the state you are affecting is a pre-release state.

The flip-side may be that doing accidental pre-releases isn't too bad, alongside doing accidental real releases when you meant to do a pre-release is unlikely.

So maybe it's inflicting pain for no actual benefit to fork the commands.

@emmatown
Copy link
Member Author

separate command comments

What do you think of loudly saying "you're in prerelease mode, a prerelease is happening" in version + publish to mostly mitigate this?

Another concern of separate commands is, what does get-release-plan do? Does it throw? Accept an option? Does that mean you have to check if a repo is in pre mode before getting a release plan?

@Noviny
Copy link
Collaborator

Noviny commented Oct 10, 2019

What do you think of loudly saying

This seems mostly fine, but a) so loud we use dominik's letter package, and b) doesn't help if logs are hidden by automation.

what does get-release-plan do?

There's a general question with that: In pre: do you run applyPre(getReleasePlan()) or getReleasePlan({ pre: true })?

Since pre affects versioning, and likely requires another cascading iteration the way linked does, I think it's probably the second one, which means you need to know your pre/not-pre state before you begin it.

This is already implied by the fact you're putting pre into assemble-release-plan

@emmatown
Copy link
Member Author

doesn't help if logs are hidden by automation.

I'm thinking we can add a warning to the PR body that the release action creates.

There's a general question with that: In pre: do you run applyPre(getReleasePlan()) or getReleasePlan({ pre: true })?

I think the answer is getReleasePlan(). The reasoning being that doing a non-pre release when in pre mode would break things.

@Noviny
Copy link
Collaborator

Noviny commented Oct 10, 2019

I'm thinking we can add a warning to the PR body that the release action creates.

I'm happy with this mitigation.

getReleasePlan()

:this-tbh:

assembleReleasePlan should take it as an option, but yeah, getReleasePlan should take in cwd at most.

@Noviny Noviny mentioned this pull request Oct 10, 2019
Copy link
Collaborator

@Noviny Noviny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a non-blocking nit.

Another thing to call out here:

Our 'should publish' check in publishPackages.js on line 102 is semver.gt(localVersion, publishedVersion)

This once again isn't blocking, but particularly if you're running two release branches, some of your pres could fail to publish because the main branch got ahead of them. I think altering the publish script could be a followup issue.

(note this also stops back-fixes in changesets as well which people have asked for)

// we want to make the prerelease version(the number at the end) across all packages the name
(preState === undefined || preState.mode === "exit"
? ""
: `-${preState.tag}.${preState.version}`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this not to be the case, mostly for random missed versions, but it probably doesn't matter.

@emmatown
Copy link
Member Author

One last problem/maybe it's fine:

I have pkg-a@1.0.0.

I enter pre with tag next.

I add a changeset with pkg-a@patch

I run version so pkg-a is now at 1.0.1-next.0.

I add another changeset with pkg-a@minor.

I run version, should pkg-a be at 1.1.0-next.0 or 1.1.0-next.1? The current implementation makes it 1.1.0-next.1, is this the right thing to do?

@Noviny
Copy link
Collaborator

Noviny commented Oct 30, 2019

One last problem/maybe it's fine:

I don't want to block releasing this to niggle at this problem. I vaguely called it out above (it's the same when you do your first pre-release of a package if there's been other pre-releases already). If we need to solve this, we can store more info, be more clever, but I'd only do that if we discover this is a genuine problem.

@Noviny
Copy link
Collaborator

Noviny commented Oct 31, 2019

image

This error appears when you are running the version after exiting, which is a bit misleading/not ideal.

I ran some manual tests with the version command (I'm pretty comfortable with not testing changes to publish).

Current thought: I might merge and release the current version branch, then we merge this in immediately after and release (just making sure those changes are out in case this breaks anything). Then we can have fun from there!

@emmatown emmatown merged commit 8f0a1ef into master Oct 31, 2019
@emmatown emmatown deleted the prerelease branch October 31, 2019 06:19
@github-actions github-actions bot mentioned this pull request Oct 31, 2019
@ASISBusiness
Copy link

View the rendered doc

Closes #9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prereleases
3 participants