-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat(monorepo): setup monorepo skeleton and break js to 2 packages #844
Conversation
This is looking good! I'm reviewing on the monorepo branch. I also just learned that Lerna is being maintained again 🎉. For testing the versioning/publishing process, what if you set up a test NPM org that you could publish the packages under? |
cc @shalvah |
Indeed! This helped me decide to go with Lerna, because even if the monorepo evolves and we'd need more advanced features, we can always migrate to Nx. With Lerna, we have all we need at the moment with very little learning curve.
I am going to add all of these into the
Good idea, I will look into that. |
Nice, nice! |
# Conflicts: # CHANGELOG.md # package-lock.json # package.json # packages/js/src/server/transport.ts
Released under @hb-test org! |
@joshuap I replaced shipjs with
All of the above are done with On the same topic, I haven't made up my mind yet about scheduled deployments. I'm not a big fan of the current process, because if there's a PR from scheduled deployment, I can't simply trigger a newer manual deployment; I must first close the scheduled deployment PR, delete the release branch and then trigger the manual deployment. |
@joshuap @shalvah I think this is ready.
|
I'm good with whatever you prefer. My primary concerns are:
One question—does the new release command also release to the CDN, like the current one does? |
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.
Looks good here, all the local commands worked for me. 👍
I will let @shalvah approve.
Will check this out today or tomorrow. @subzero10 I think the last merge caused some conflicts. |
Gotcha, makes sense. Then I guess we have to enable scheduled deployments. Can we do this right after we merge this PR? Or is this a blocker for you? The reason is that I have tested this branch heavily and I'd rather merge it before I do further work on it.
I think this part is handled, but the vocabulary may need improvement. I used the word automation to refer to scheduled/automatic releases, which is something we don't have (at least for the beginning; see the point above). Can you please review this part of the documentation*? Basically, a new release will be possible with the click of a button from the Github Actions page. *Note: you might need to wait a couple of seconds for the link above to load since it has to load all the file changes.
Yes, the release to the CDN is executed as a |
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.
Nice! Well done with all the work on this. Looks good. The core + js structuring makes a lot of sense.
I left a comment about the README, but I know you've already spent a lot of time on this, so I'm pushing the needed changes myself.
One thought I have (which is probably a separate discussion from this PR): I think we shouldn't enforce Conventional Commits locally. I think we should only enforce it at the PR level (which is what matters, since the commits get squashed anyway).
Rationale: I get the usefulness, but it can often be an unnecessary annoyance, especially for outsiders. I've contributed to many open source repos, and the most painful ones were those that had very specific rules you needed to follow, even for a one- or two-line docs change. The best ones avoided majoring on the minors—if you can easily fix it at the final point, don't put the burden on contributors. In this case, the fix is as simple as renaming a PR. Wouldn't that be enough to get a good changelog?
I agree with this. Basically, enforce conventional commits on main, but not on other branches, right? 👍 |
OK, no objection from my side. I don't know how to do that though. And it probably means that we have to manually come up with the squashed commit message. On top of that, there are a couple of challenges. Usually, we merge from github's web interface. Github has a default strategy for the merge message, which depends on the PR title and number of commits:
I think the whole idea of conventional commits is not for the changelog generation, but the practice for meaningful commits. Changelog generation is a consequence of this practice. What we are suggesting now is to avoid this practice but try to use it for changelog generation. Nevertheless, I totally get your point, the repository is open source and we shouldn't enforce too many policies. So:
|
Yes, exactly.
That's the theoretical idea, but remember that conventional commits is a specific, opinionated style of committing. Personally, I write meaningful commits by following a simple rule: describe what this commit does. Enforcing conventional commits doesn't equate to enforcing meaningful commits. Nothing stops someone from writing a commit message like
Yes, but this is actually pretty simple. When merging a commit, GitHub shows a text box (after you click the merge button the first time) where you can edit the merge commit message (defaults to the PR title). It's a tiny bit of work to edit it, but it's negligible, really, and something I do often in certain codebases. Yes, the responsibility is shifted onto us, but you'll see it's a really, really small task. I know you're skeptical, but thanks for giving it a try.😀 Re: implementation, I think the best option is to NOT enforce conventional commits on any branch (because enforcing on main is kinda pointless, since it won't stop a PR merge via GitHub), but use it to generate changelogs still. Basically, trust us to set the right commit message when merging. If we really need to enforce, we could enforce the PR title, if that's possible. |
Status
READY
Description
Monorepo features
todo
ci - scheduled deployment