restructure repo to publish types properly to npm registry

[sumeet-bansal]

Published types should only have required dependencies and we should limit the manual work required to version properly.

I imagine this would take the form of a multi-package repo managed by lerna or some similar tool, and possibly some semantic release tool like semantic-release kicked off by our CI pipeline.

[sumeet-bansal]

@shravanhariharan2 gonna tackle this next since Sage Team and Dimensions are going to be using our API. Thoughts?

Our goals:

• only publish our API spec and types (maybe even a full API wrapper for convenience at some point) with no implementation dependencies
• publish with correct versions
• encourage documenting API changes

I looked into some existing tools but they don't play well with lerna and weirdly rely on commit message structures which seem pretty liable to break since it's hard to enforce that on PR titles. But the general idea for them's that you commit to master and format the commit message in a parseable way, then it'll bump the version, commit again to master, and publish to npm and GitHub for you. The way I outlined below requires slightly more legwork upfront but is a little cleaner I think because it lets us update versions in their respective PRs instead and still publish npm and GitHub releases without enforcing any rules on commit messages.

For publishing:

• restructure the repo as 2 separate packages (the API spec and the API implementation) using lerna, maybe a third package for tests
• configure lerna to only publish the API spec package
• developers still have to manually version (can be done for both packages through lerna version, can set set up an npm script for this)
• CI check (manual approval) where developers confirm they set the version if needed
• CI check (manual approval) where developers confirm they documented their API changes if needed
• CI job to publish npm release: only on master, lerna publish
• CI job to publish GitHub release: only on master, might be able to do this with GitHub CLI or github-release-cli or something similar

Long-term it'd be nice to automate versioning with a GitHub bot or something (it comments on your PR with a checklist like ["improvement";"patch";...], you check a box, and it commits your version bump), and auto-generate docs from code like TypeDoc but for Postman, but those aren't important right now.

[shravanhariharan2]

Couple comments/questions:

general idea for them's that you commit to master and format the commit message in a parseable way, then it'll bump the version, commit again to master, and publish to npm and GitHub for you

Are two commits necessary? And how are they supposed to be formatted (and is that taken care of by the CI before merge)?

restructure the repo as 2 separate packages (the API spec and the API implementation) using lerna, maybe a third package for tests

Thoughts on also adding seeds/fakes as a package? Not sure how this would work since a lot of it requires implementation functionality, but a developer could work directly with portal state programatically. Not sure how useful this might be but let me know your thoughts.

CI check (manual approval) where developers confirm they documented their API changes if needed

Not sure what you mean by manual approval for check - does this appear in CircleCI as a task with no actual job to run?

Also, this documentation refers to our Postman docs right? Right now that's only accessible from our ACM development account, [email protected]. I temporarily moved it there since free Postman only allows 25 shared requests (it prevented me from adding/editing any requests after that), so should we move to a paid plan ($12-15 per user per month)? Alternatively we could have everyone working on one Postman account to make it free free but thats a huge hassle to manage credentials

Also, how does it work of two people have different PR's that both need to bump versions? You mentioned that we can update versions within the PR, but this might cause conflict with other PR's right? Or is that bump only on merging, where the other PR can merge those changes & bumps and bump their own after that? But then wouldn't they be blocked by the other PR merging first before they version bump?

Overall, it seems pretty clean but a little complicated of a change just to add versioning for types/specs, but looks like this will help down the road w/ documentation + API wrappers and it doesn't look like there's any cleaner & automated alternatives.

[sumeet-bansal]

1. I think some can amend commits but git history should be immutable, so I'm not considering that an option, although I guess it's very unlikely to cause conflicts since the amend happens within a minute of merging. Since the commit messages are the PR titles, I don't think that's something CI can handle?
2. Seeds and fakes would go in the testing package. Packages can use each other, so implementation would use types and tests would use implementation and types.
3. Manual approval means it's a CircleCI task with no job, so in the list of PR checks you click on the approval checks and click yes/no (or something like that). See here for details on CircleCI manual approval.
4. Yes, our Postman docs. Postman isn't worth paying $12-15/user/month, and we already have single accounts for Heroku, Netlify, etc.
5. If one PR merges a version bump, it should cause a merge conflict in another PR with its own version bump since they're both making changes to the same lines.
6. Yeah it's a good bit of work but it's not really for versioning so much as it is for publishing our API specs for clients like UI, Sage, and Dimensions to use. Since I'm sure other teams are going to be building off the portal in the future, we should do publishing right, and versioning is a part of that.

[sumeet-bansal]

@shravanhariharan2 any other thoughts on this?

[shravanhariharan2]

1. So do PR titles need to be formatted a specific way? Like how would those messages be formatted / is that something we need to standardize in PR titles?

Rest sounds good to me

[sumeet-bansal]

PR titles don't need to be specifically formatted, but I'll switch to uppercase imperative to match your PRs since that's more common.

[shravanhariharan2]

ok, and are we trying to match commit formats too? Like imperative vs. past, upper vs. lower, or does that not matter since the PR title is what gets commited to master anyway?

[sumeet-bansal]

Commits in PR branches don't matter.

[shravanhariharan2]

@sumeet-bansal @michl1001 there's been some interest in getting this set up within the next few weeks for ease of development in frontend for merch store (plus other QoL things for frontend with the API wrapper). Since we aren't necessarily working on any features currently, I'm looking to dig deeper into what we want out of a restructure like this.

Some questions to consider / discuss:

• Are we waiting for anything else that's blocking this restructure? I imagine route param validation is one of them but beyond that was there anything else we should get finished before this? Things that come to mind are seeds, testing, docs, etc.
• Are we going to stick with lerna for the restructure? I haven't used it myself but by looking into it a bit, it seems like it'll get the job done but I'm wondering if there's anything you both know of that might do the job better
• How are we looking to restructure the repo? Something like:

packages/
    types/
    api/
    seeds/
    impl/(?)
    ...

initially might work but I'm not sure what's the best way to split implementation from spec in a package sense.

I think we should also definitely consider writing the API wrapper now as well since it makes testing a lot easier. As I've been working more on testing I'm realizing how much easier it'd be to have actual API calls rather than directly using the controller methods, since things like validating multer storage and other types of validation are either difficult or are just not possible to do with the current setup.

[sumeet-bansal]

• Route param validation isn't really blocking this. It'd be nice to have more testing before making the change but I don't think that's blocking either.
• lerna should be fine.
• Packages: api, impl, tests; seeds and migrations are kinda odd to place but maybe seeds can go under tests and migrations go under impl?

I think our first goal's to publish types, and the API wrapper can be done afterwards.

What can't we test with our current setup?

[shravanhariharan2]

One thing I'm unable to test now is if an error is thrown if a file passed in is of too large of a size to the upload controllers (e.g. event cover upload). From what it looks like a lot of the processing/validation happens under the hood of @UploadedFile and isn't testable with class-validator, so I don't think we can test the "too large files" assertion.

If that's a minor thing (which is what it seems like) then I can just continue writing some more tests to cover the essential things (events and attendance) before moving forward.