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

Split components into seperate packages #411

Merged
merged 26 commits into from
Apr 2, 2021
Merged

Conversation

wolfy1339
Copy link
Member

As per our discussion in #362, I started refactoring the repository to move the 3 components of this repo (examples, schemas, types) into seperate packages.

All the build scripts work, all test scripts work except for the validate script

@wolfy1339 wolfy1339 added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Mar 21, 2021
@wolfy1339 wolfy1339 changed the title 🚧 Split components into seperate packages 🚧 Split components into seperate packages Mar 21, 2021
@wolfy1339 wolfy1339 requested review from G-Rath, gr2m and oscard0m March 21, 2021 20:33
package.json Outdated Show resolved Hide resolved
@wolfy1339 wolfy1339 linked an issue Mar 21, 2021 that may be closed by this pull request
@wolfy1339 wolfy1339 changed the base branch from master to beta March 21, 2021 22:51
@wolfy1339 wolfy1339 changed the base branch from beta to master March 21, 2021 22:52
@wolfy1339
Copy link
Member Author

Everything now works for build & test scripts.

Now to figure out how to get semantic-release to publish the 4 packages at the same time

@wolfy1339 wolfy1339 force-pushed the split-multi-package branch 2 times, most recently from f8df726 to 6e55714 Compare March 29, 2021 20:49
@wolfy1339 wolfy1339 force-pushed the split-multi-package branch from 6e55714 to a9aaa06 Compare March 31, 2021 20:54
@wolfy1339 wolfy1339 requested a review from G-Rath March 31, 2021 20:55
Copy link
Member

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks good to me

@@ -3,7 +3,7 @@
import { DefinedError, ErrorObject } from "ajv";
import path from "path";
import { inspect } from "util";
import { ajv, validate } from "../payload-schemas";
import { ajv, validate } from "../payload-schemas/index";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { ajv, validate } from "../payload-schemas/index";
import { ajv, validate } from "../payload-schemas";

nit: node imports index if it exists when you tell it to import a folder, meaning you can leave this file as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it usually does, but in this case, since there's a package.json in the directory with a main property, it'll use the main property which in this case is schema.json.

Which is why I used an explicit import instead of implicit

@wolfy1339 wolfy1339 changed the base branch from master to beta April 1, 2021 21:48
@gr2m
Copy link
Contributor

gr2m commented Apr 1, 2021

Also it looks like the README has not yet been updated.

What should the 3 packages be? Can we put them in 3 different directories, and have a README.md file for the respective package so that it will show up on npmjs.com?

@wolfy1339
Copy link
Member Author

Also it looks like the README has not yet been updated.

I'll update the README

Can we put them in 3 different directories

They already are seperated, only the global package is

payload-types => @octokit/webhooks-types
payload-schemas => @octokit/webhooks-schema
payload-examples => @octokit/webhooks-examples
./ => @octokit/webhooks-definitions

and have a README.md file for the respective package so that it will show up on npmjs.com?

For sure, that can be done.

@wolfy1339 wolfy1339 force-pushed the split-multi-package branch from 98e1ee2 to d7ff69a Compare April 1, 2021 23:09
@wolfy1339
Copy link
Member Author

I have found this semantic-release plugin, https://github.com/pmowrer/semantic-release-monorepo
It does look like it could very well help us out

@gr2m
Copy link
Contributor

gr2m commented Apr 2, 2021

I have found this semantic-release plugin, https://github.com/pmowrer/semantic-release-monorepo
It does look like it could very well help us out

I didn't use that plugin myself, but it sounds like it supports different versions for the different packages in the repository, which is not something we need in our case.

I think we could use semantic-release for the root (@octokit/webhooks-definitions), use semantic-release-plugin-update-version-in-files to replace the versions in the payload-*/package.json files, then run npm publish in the payload-* folders. That we we'd have a single version for all packages in this repository, with one matching github release and its release notes.

@wolfy1339
Copy link
Member Author

wolfy1339 commented Apr 2, 2021

The changes are made.

Although there won't be a way to tell npm that this should be a beta, currently EDIT: I found a way to set the beta tag whenever the branch is beta

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@wolfy1339 wolfy1339 merged commit 92cca6a into beta Apr 2, 2021
@wolfy1339 wolfy1339 deleted the split-multi-package branch April 2, 2021 21:46
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 3.67.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 3.68.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wolfy1339
Copy link
Member Author

wolfy1339 commented Apr 2, 2021

It appears as though, it's not publishing the other packages.

The steps get skipped over EDIT: The steps now get fired properly.

@wolfy1339
Copy link
Member Author

There is 2 issues here currently:

  1. The packages don't seem to want to be published to npm (possibly due to the fact that we are trying to use the @beta tag when @latest doesn't exist yet
  2. semantic-release-plugin-update-version-in-files doesn't seem to be working, there is no mention of it in semantic-release logs

@gr2m
Copy link
Contributor

gr2m commented Apr 2, 2021

I’ll have a look ASAP, tonight or tomorrow

@wolfy1339
Copy link
Member Author

I don't think this solution for publishing packages will work as is.
Currently it will publish a release on every single push to either the master or beta branch. What would be needed is logic similar to the one in @semantic-release/commit-analyzer to determine if we need to make a release

@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 3.68.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving index.json to a different package
4 participants