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

feat!: 2024 updates, maintainer handoff proposal #29

Merged
merged 24 commits into from
May 7, 2024

Conversation

broofa
Copy link
Contributor

@broofa broofa commented May 6, 2024

@lukekarrys : Hey Luke, given the lack of action on #18 (comment), would you be willing to let me step up as maintainer here?

I know that might be a big ask, but this repo is (or at least should be) a valuable resource for clients of the registry API.

FWIW, this PR shows what I'd like to see happen with this project, at least initially, to bring it up to date and make it easier to maintain.


Summary of changes, in no particular order:

  • Enable corepack
  • Update all dependencies to latest
  • Apply @jabilko's PRs
    @jablko put up test: check actual examples from the npm registry #15, chore: extract common declarations #16, chore: use the record type #17, feat: update types #18, and ci: test pull requests #19, all of which should have been merged long ago. To the extent feasible, they've all been incorporated here.
  • Remove gts dependency
    gts is a thin wrapper around eslint and prettier, with google-opinionated settings that are no longer relevant. Using eslint and prettier directly is makes these tools easier to manage and update.
  • Remove pacote dependency
    pacote is a level of abstraction that is not helpful. These types should accurately document what the registry endpoints provide, without possible munging by intermediate code like pacote or it's dependencies. Using node's native fetch() is simpler and easier.
  • Additional fixtures from registry
    The most important facet of @jablko's work was the addition of "fixtures" unit tests to verify registry requests actually validate against the TS types. This is a huge improvement. In addition to the endpoints @jablko set up unit tests for, I've added a few others that surfaced some issues with the types. Assuming I continue to maintain this, my plan would be to have any future changes to types be accompanied by fixtures that demonstrate how/where types are used / not used.
  • Fix existing types issues. index.d.ts validates against all fixtures.

@lukekarrys
Copy link
Contributor

The biggest blocker to #18 was onboarding this repo to our template-oss tool which I think had just recently got typescript support when I commented on the PR.

I just landed that in #31 which should allow us to more easily merge this PR. Would you be willing to fix the new conflicts I just created?

I can't speak to transferring maintainership right now, but I would like to get this PR merged.

@lukekarrys
Copy link
Contributor

We try to control all our linting and configuration via template-oss so those changes will need to be backed out. But all the types and test updates look good to land.

@broofa
Copy link
Contributor Author

broofa commented May 6, 2024

@lukekarrys Thanks for the quick response! I'll take a look at the conflicts this evening and update.

@broofa broofa changed the title Update for 2024, maintainer handoff proposal feat!: 2024 updates, maintainer handoff proposal May 7, 2024
@broofa
Copy link
Contributor Author

broofa commented May 7, 2024

@lukekarrys While updating the README, I realized there were a number of maintenance headaches here. 'Figured this would be a good opportunity to clean up the following:

  • Remove the PackageLock and LockDependency types. Lockfiles are not present in the registry API. Hence, those types aren't appropriate here. (From the README: "Typescript definitions for npm registry content")
  • Rename PackageJson -> PackageJSON. Possibly controversial, but this looks "more correct" to me, and aligns with the best source I could find for acronyms-in-variable-names (AirBnB style guide).
  • Remove all other subtypes from exported properties. My experience has been that exporting internal convenience types like Maintainer and Directories invariably ends up being a maintenance headache. Having users depend on these makes refactoring more difficult. Users that want such types can create their own using indexed access types. For example:
type Maintainer = NonNullable<PackumentVersion['maintainers']>[number]; // <-- resolves to the internal  `Contact` type

('still working on resolving other oss-template issues, btw.)

@broofa
Copy link
Contributor Author

broofa commented May 7, 2024

@lukekarrys Okay, so I've fixed the merge conflicts, updated the README, and made various other fixes and ✌️improvements✌️... all while doing my best to ignore template-oss-check grumbling. While I see the value in template-oss` - especially where GH actions and the release scripts are concerned - it does seem to be a bit of an awkward fit for this particular project.

Since it looks like template-oss is a new project and you're the main contributor(?), I'd like to push back a little. Hopefully this is helpful...

From the template-oss-check log...


  tsconfig.json
  ========================================
  "compilerOptions.declaration" is missing, expected true
  "compilerOptions.declarationMap" is missing, expected true
  "compilerOptions.jsx" is missing, expected "react"

tsc is only used here for compiling fixtures.test.ts, which is dev-only. There's no point in generating declaration files or maps. They're just filesystem clutter (in this context). Maybe only require these settings if project has a src directory?

'Not using jsx. Omiting this would be nice. TS config files are confusing enough without adding unnecessary options. Maybe only require this if a project has jsx or tsx files?

  .eslintrc.cjs
  ========================================
  ...
  -  rules: {
  -    'max-len': 'off',
  -  },
  -  extends: ['@npmcli', ...localConfigs],
  +  extends: [
  +    '@npmcli',
  +    ...localConfigs,
  +  ],
   }

I'd like to format the fixture files, but they contain SHA hashes (90+ chars), which eslint complains about (max-len rule). The simple solution would be to disable that rule but that's not allowed.

For now I've added an .eslintignore file to just ignore the whole "test/fixtures" directory, but that's not ideal. I would prefer to have those formatted.


  .gitignore
  ========================================
  @@ -28,5 +28,4 @@
  ....
  -!/types/

The only file this module publishes is types/index.d.ts. Putting such files in a "types" dir is common practice. I could move it up to the project root, but that requires changing package.json#files, which just produces a different error.

template-oss-check wants files = ['dist'], but index.d.ts isn't a generated file, so having it in the dist directory feelswrong.

  package.json
  ========================================
  ...
  "scripts.prepare" is missing, expected "tshy"
  "scripts.snap" is missing, expected "tap"
  "scripts.test" is "node --test --enable-source-maps", expected "tap"
  "tap" is missing, expected {
    "nyc-arg": [
      "--exclude",
      "tap-snapshots/**"
    ]
  }

tshy doesn't make sense here. We're not publishing ESM or CJS code, so I don't think tshy applies here. If you look at other types-only projects ("node_modules/@types/*"), they omit main and exports fields from their package.json completely.

Also, tshy throws since there's no "src" directory. 🤷

Regarding tap...

  • Adds 295 dependencies with 165 MB to the `node_modules footprint.
  • Rather obscure (I hadn't heard of it I don't see it mentioned when googling "javascript testing framework" lists)
  • Contributes to testing framework fatigue. Mocha, Jasmine, Ava, Jest, Vitest...

I get that node:test isn't the be-all-end-all of frameworks, but it is at least built into node and it works great for "small" use cases like this.

The following files are tracked by git but matching a pattern in .gitignore:

  .vscode/tasks.json

Can't complain too much about this one, but having VSCode set up to run tsc --watch is a nice-to-have. Pairing that with node --test --watch (in a separate terminal) means unit tests run locally automatically.

@broofa
Copy link
Contributor Author

broofa commented May 7, 2024

whew... in summary, as a prospective maintainer (maybe? possibly?) I'm leery of the template-oss stuff in it's current form. It feels a bit heavy-handed, with a lot of overhead that's not terribly relevant or that may actively interfere with DX.

I like the idea of tooling that helps projects set up a standard project structure, but would prefer to see it presented in a more wizard-like fashion, so I could opt into things as-needed.

This project appears to have JSX. Add jsx: "react" to tsconfig.json? [Yn]

Set up project for dual ESM/CJS module support with tshy? [Yn]

Set up GH actions for code quality? [Yn]

Add "dist" to package.json#files? [Yn]

etc.

Similarly, having template-oss-check be a bit more gentle about what it requires would be nice. Maybe have it distinguish between hard requirements (CODE_OF_CONDUCT) vs. "suggestions" (contents of ".gitignore")?

@lukekarrys
Copy link
Contributor

To give a little perspective, @npmcli/template-oss is designed to allow our team to maintain a bunch of open source packages. I'm the primary maintainer and it's pretty much designed solely for the npm team's OSS use case, where one of the biggest factors is making a bunch of different repos act as similarly as possible. As you've mentioned above this comes with a lot of tradeoffs. Some of the things you mentioned currently have "escape hatches" in template-oss, others might be fixed by adding new escape hatches, and others might just be shoehorned into this repo even if its not a perfect fit.

Using @npmcli/template-oss is a requirement (made by me 😄) for open-source repos to be considered "maintained" by our team. I fully expect that if we were to transfer a repo or hand over maintenance that all those bits would be ripped out by the new maintainers.

Since a conversation about maintainership would take longer and possibly get blocked (mostly due to our team not having done that before), so my goal here to get all of these long awaited fixes finally merged and published. I think the most helpful thing would be to get a list of any true blockers that template-oss is imposing.

From the list above I see:

  • Remove tshy since that isn't designed for a types only package

Is there anything else you think will make it impossible to publish the new types changes and tests?

@broofa
Copy link
Contributor Author

broofa commented May 7, 2024

@npmcli/template-oss is designed to allow our team to maintain a bunch of open source packages

I 100% understand. Supporting opensource is difficult and time-consuming. You gotta scale it somehow.

Is there anything else you think will make it impossible to publish the new types changes and tests?

Impossible? No... I don't think so.

Can you clear up the remaining template-oss issues or do you need me to do that? I could take a stab at it, but if there's template-oss work involved it probably makes more sense for you to take over for a bit.

@lukekarrys lukekarrys merged commit 123bb67 into npm:main May 7, 2024
6 of 7 checks passed
@github-actions github-actions bot mentioned this pull request May 7, 2024
@lukekarrys
Copy link
Contributor

@broofa I was able to get all the template-oss stuff cleared up. I also refactored the tests slightly so the fixtures are stored as snapshots. I thought it would be good to validate both that they match the types and separately be able to assert when they change.

Very open to any more followup PRs you have! Thanks for your work on this.

@broofa broofa deleted the jabiko_patches branch July 1, 2024 12:49
reggi pushed a commit that referenced this pull request Aug 13, 2024
🤖 I have created a release *beep* *boop*
---


## [2.0.0](v1.0.2...v2.0.0)
(2024-08-07)

### ⚠️ BREAKING CHANGES

* update types (#29)
* refactor to use @npmcli/template-oss (#31)

### Features

*
[`123bb67`](123bb67)
[#29](#29) update types (#29) (@broofa,
@jablko, @lukekarrys)
*
[`f09f754`](f09f754)
[#31](#31) refactor to use
@npmcli/template-oss (#31) (@lukekarrys)

### Bug Fixes

*
[`6ffee7f`](6ffee7f)
[#60](#60) add missing fields, fix up
array type, add comments (#60) (@broofa, @styfle)
*
[`4548f2c`](4548f2c)
[#39](#39) linting cleanup (#39)
(@lukekarrys)

### Documentation

*
[`5511d4b`](5511d4b)
[#40](#40) fix README typo, improve
PackageJSON description (#40) (@broofa, @styfle)
*
[`1a08144`](1a08144)
[#37](#37) fix typo in readme (#37)
(@lukekarrys)

### Chores

*
[`d323311`](d323311)
[#70](#70) bump @types/node from
20.12.10 to 22.1.0 (#70) (@dependabot[bot])
*
[`4761562`](4761562)
[#71](#71) bump
@typescript-eslint/parser from 7.18.0 to 8.0.1 (#71) (@dependabot[bot])
*
[`3687a18`](3687a18)
[#38](#38) simplify template-oss config
(#38) (@lukekarrys)
*
[`1a1fd85`](1a1fd85)
[#36](#36) remove build script and
update snapshots and tsconfig (#36) (@lukekarrys)
*
[`3172a32`](3172a32)
update template-oss files for main branch (@lukekarrys)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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