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

Add the ability to upload to @types/x #1025

Merged
merged 4 commits into from
Jun 25, 2021
Merged

Add the ability to upload to @types/x #1025

merged 4 commits into from
Jun 25, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented Jun 22, 2021

Supports mapping the generated files from the generator to different @types packages via a deploy process. I ended up more or less using the exact same infra as tsconfig/bases but ported from Deno to Node.

Roughly, as you'll see it during code review:

  • Adds a yml for deploys on master merges. Runs through building and tests, then creates packages and deploys.
  • deploy/createTypesPackages.mjs is responsible for generating all possible packages as complete artefacts
  • deploy/deployChangedPackages.mjs is responsible for comparing the built packages against the versions on npm, if they differ - then deploy

TODO:

  • I probably want the ability to have a custom README for @types/web (at least) but maybe I just vendor each README in the repo so they can can hand crafted to better describe their domain.

  • I'm still a little undecided on what should be done with the .iterator files. E.g. should they be included with the corresponding non-iterator. I lean on yes, but I need to verify with others.

Iterators are included by default in pretty much every case but ES5:
 
Screen Shot 2021-06-22 at 2 19 35 PM

@orta orta requested a review from sandersn June 22, 2021 13:22

// Loop through generated packages, deploying versions for anything which has different
// .d.ts files from the version available on npm.
const generatedDir = join(__dirname, "generated");
Copy link
Contributor

@saschanaz saschanaz Jun 22, 2021

Choose a reason for hiding this comment

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

Passing URL value to fs methods can skip the path conversions.

Edit: (Like this) https://github.com/saschanaz/types-web/blob/7b2675fc37d568d2d99e288eb63125f8c55581af/src/version.ts#L7

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Mostly questions about error handling.

I vote to have custom READMEs, since I don't think we'll have that many different packages (at first, especially).

deploy/createTypesPackages.mjs Outdated Show resolved Hide resolved
deploy/deployChangedPackages.mjs Show resolved Hide resolved

// Publish via npm
if (upload) {
if (process.env.NODE_AUTH_TOKEN) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a missing NODE_AUTH_TOKEN should mean failure, or printing a dry-run message like "Dry run: would have uploaded ${name} with 'npm publish --access public ...' "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do log at the end that it was a dry run, so I think this is probably fine, but I can expand on the logs for the dry run.

Copy link
Member

Choose a reason for hiding this comment

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

👍 more logs are good

deploy/deployChangedPackages.mjs Show resolved Hide resolved
deploy/createTypesPackages.mjs Show resolved Hide resolved
deploy/createTypesPackages.mjs Show resolved Hide resolved
deploy/template/README.md Outdated Show resolved Hide resolved
@saschanaz
Copy link
Contributor

* is responsible for comparing the built packages against the versions on npm, if they differ - then deploy

Can we do this locally by comparing between git tags in baselines/?

@orta
Copy link
Contributor Author

orta commented Jun 23, 2021

Can we do this locally by comparing between git tags in baselines/?

Probably not if we want to be able to deploy just @types/web-sharedworker and not @types/web when only shared worker has changed, tags would only work if all version numbers were kept in sync

@saschanaz
Copy link
Contributor

Probably not if we want to be able to deploy just @types/web-sharedworker and not @types/web when only shared worker has changed, tags would only work if all version numbers were kept in sync

Webref have tags for each package https://github.com/w3c/webref/releases and I wonder it can also be done here. This way we can find the latest tag and see if the corresponding file changed from that tag.

@orta
Copy link
Contributor Author

orta commented Jun 23, 2021

Nice, scoping the tags via a name seems pretty reasonable to me.

Should be pretty easy to get access to the raw files for a release via the GH API 👍🏻

@saschanaz
Copy link
Contributor

FYI this is what I'm doing on types-web: https://github.com/saschanaz/types-web/blob/7b2675fc37d568d2d99e288eb63125f8c55581af/src/changelog.ts#L5-L13

git describe --match seemingly also supports filtering by tag name👍

@orta
Copy link
Contributor Author

orta commented Jun 23, 2021

Using git to make the comparison won't realistically work without a bunch of git faff on every push, which I don't think is worth it - we'd be comparing the files which are meant to be .gitignored, so we'd have to have CI ship a version with the gitignore changed, then undo it.

I think we're fine to use unpkg, but I'll make the release tagging (and once we're up-to-date with types-web, I'll make the CHANGELOG feed into that in GitHub releases)

@saschanaz
Copy link
Contributor

Could you elaborate why it would do anything with every push?

@orta
Copy link
Contributor Author

orta commented Jun 23, 2021

I've been using the files in the npm package as the source of truth and both the npm packages aren't included in the git repo nor are the files which they use in generated.

I could maybe go backwards through the generator metadata object get back to the originals, and use the baseline test files instead - but this way means comparing the exact same file in the package to the exact same file package.

A safe alternative which can be relied on it in git is:

  • Force add the generated packages into git
  • Commit that with a [no ci], use this for the tag reference
  • Make another [no ci] commit undoing the generated packages

Which would mean for every PR merge it adds x new commits also.

But that's more effort than just downloading and unzipping the npm tarball and looking at the raw file contents instead of relying on unpkg to do that for us, I don't have to worry about unzipping tarballs on windows this way

@orta
Copy link
Contributor Author

orta commented Jun 23, 2021

I've asked in the TypeScript chat room about including dom.iterator.d.ts in types/web - I'm on the site of "yes", we'll see -this can be un-blocked before then. Given it's all deploy stuff, I can only know it works when it hits reality.

@orta orta marked this pull request as ready for review June 23, 2021 13:47
@saschanaz
Copy link
Contributor

I've been using the files in the npm package as the source of truth and both the npm packages aren't included in the git repo nor are the files which they use in generated.

Are you going to use git diff, because otherwise the files don't have to be in the git repo? Files can be loaded via fs.readFile/git show and then be ===ed instead.

@saschanaz
Copy link
Contributor

Anyway, I guess it can be a separate work if the current solution works.

@orta
Copy link
Contributor Author

orta commented Jun 25, 2021

I think we need to keep the 'if no package exists, then deploy' logic - none of us are logged in as the types account on npm to initially add the dependency

@orta orta merged commit e394e61 into master Jun 25, 2021
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.

3 participants