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

Feature/versioning restructure #28

Merged
merged 13 commits into from
Aug 18, 2022
Merged

Conversation

benjaminbojko
Copy link
Collaborator

Creating this PR to discuss how we merge this in. I like following established best practices here, esp since we've been having issues with installing this on Mac OS. Primarily, there seem to be install issues when there are file:// dependencies in the package.json (Cannot set properties of null (setting 'dev'), see npm/cli#3847 (comment)).

Will do some testing and deeper reviewing and post updates here.

@benjaminbojko benjaminbojko self-assigned this Aug 3, 2022
@benjaminbojko benjaminbojko marked this pull request as draft August 3, 2022 19:44
@claytercek
Copy link
Member

@benjaminbojko do you have any thoughts on using github actions for auto-versioning/publishing?

I'd be happy to set that up we want to go for it

@benjaminbojko
Copy link
Collaborator Author

@claytercek love that idea and thanks so much for doing the dep cleanup. Could you elaborate what you think would make sense to automate here?

Also, dependencies were so fragile back when I originally put this structure together; have you tested the following scenarios with your new setup and the latest nodejs/npm?

  • Install launchpad as local dependency
  • Install individual launchpad workspaces as dependency (e.g. @bluecadet/launchpad-content)
  • Install all launchpad dependencies from within the launchpad directory for development/testing

@claytercek
Copy link
Member

@benjaminbojko The easiest way to set up automation is using the first-party github action. This will track the changesets merged into the base branch, and create a PR with the accumulated changelogs and version number updates. When we're ready to release, we just merge that PR in and it will update all version numbers and deploy all updated packages to NPM.

They also have a bot that can enforce PRs having at least one changeset.

More info here!


I've tested the following three scenarios and they are all working! I did have to make some changes to make the sub-packages executable, namely adding a bin property to their package.json files and adding a relativePaths arg to their launchFromCli calls (both added in commit cabc007).

  • Install launchpad as local dependency
  • Install individual launchpad workspaces as dependency (e.g. @bluecadet/launchpad-content)
  • Install all launchpad dependencies from within the launchpad directory for development/testing

@benjaminbojko
Copy link
Collaborator Author

This sounds dreamy, @claytercek . Would you have time to implement changeset's GH action?

What are your thoughts on main vs develop being the base branch?

And thanks for testing those other use-cases and making the edits. I hadn't checked that in a while, actually.

@benjaminbojko
Copy link
Collaborator Author

FWIW I'd also be curious about the bot, but maybe let's take one step at a time?

@claytercek
Copy link
Member

Yeah, looks like I should have some downtime this week that I can put towards this!

Ah good call about the branches, hadn't thought about that. To me it makes sense if the main branch is inline with whatever is currently published on the npm registry, while development continues to be the active development branch. I can investigate if the action is configurable for multiple branches in that way.

Agreed about the bot, it doesn't feel that necessary.

@benjaminbojko
Copy link
Collaborator Author

Perfect, thank you 🙌 really appreciate you jumping in here and lending your expertise.

Agreed that main should be the npm branch, and develop could be the latest and greatest, but possibly less tested

@claytercek
Copy link
Member

Alrighty, the github workflow should be good to go. It's set up to treat develop as the base branch, so it tracks any new changelogs committed to that branch, updating the PR whenever new changelogs are added.

Whenever we merge that PR, it will

  1. update version numbers and changelogs on develop
  2. publish all updated packages to npm
  3. merge develop into main

Right now the npm token it's using to publish is linked to my account, so it will likely say "claytercek published" these packages when this workflow runs for now. Once we have a BC installations/dev account set up in the npm org, we can swap out that token.

Also, I've added a CONTRIBUTING.md file that has some more info/instructions for future contributors on how the whole changesets/release process works.

@benjaminbojko
Copy link
Collaborator Author

Amazing, I'll take a closer look once I'm free. This all sounds super streamlined and it seems like merging this PR in will be a great test. We also have some other outstanding features and updates that need to be published on NPM (mostly in regards to Sanity).

I'll set up a central BC dev account for NPM.

@benjaminbojko benjaminbojko marked this pull request as ready for review August 16, 2022 18:55
Copy link
Collaborator Author

@benjaminbojko benjaminbojko left a comment

Choose a reason for hiding this comment

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

@claytercek everything looks great on paper--just had one question on dependencies vs devDependencies and one on the monorepo package structure that I'd like your thoughts on before I merge.

"robotjs": "^0.6.0"
},
"devDependencies": {
"dependencies": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@claytercek should these be devDependencies since these aren't needed when we install launchpad in production environments?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think it can go either way here, because any dependencies in this high-level package.json will only be used for development because this isn't actually a publishable package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about if we wanted to install this as a GH dependency? We've done this in the past to work off development builds. Would that still point to this root package.json or do you know if NPM would be smart enough to pull @bluecadet/launchpad this repo?

Copy link
Member

Choose a reason for hiding this comment

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

I'd have to do some testing to see if we could get it to work as a GH dependency, but my hunch is that it will break. However, changesets does give a couple of tools for distributing development builds:

Copy link
Collaborator Author

@benjaminbojko benjaminbojko Aug 18, 2022

Choose a reason for hiding this comment

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

Lol "Warning! Prereleases are very complicated" -- I'll take a look 🙈

package.json Show resolved Hide resolved
@benjaminbojko benjaminbojko merged commit cfddf78 into develop Aug 18, 2022
@benjaminbojko benjaminbojko deleted the feature/versioning-restructure branch August 18, 2022 23:20
@github-actions github-actions bot mentioned this pull request Aug 18, 2022
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.

2 participants