[yarn 2] Cannot find module '@octokit/core' or its corresponding type declarations

[AArnott]

While consuming this package in yarn v2, I got a compile break because this package consumes @octokit/core at runtime:

plugin-paginate-rest.js/src/index.ts

Line 8 in fb3e08d

import { Octokit } from "@octokit/core";

Yet is package.json file claims that this dependency is not needed in production:

plugin-paginate-rest.js/package.json

Lines 27 to 31 in fb3e08d

	"dependencies": {
	"@octokit/types": "^5.2.0"
	},
	"devDependencies": {
	"@octokit/core": "^3.0.0",

Yarn's PnP mode catches this error, and I have to workaround it by putting it into loose mode.

[gr2m]

It's only importing types, not code, so it's not a runtime dependency, only a build-time dependency

[gr2m]

And you cannot use the plugin without also having @octokit/core in your dependencies, so it shouldn't be a problem anyway?

[AArnott]

It is a problem because the typescript compiler looks at your code when my code depends on it, and the typescript compiler can't find @octokit/core and it fails. Yes, I depend on @octokit/core, but your package doesn't, so the reference is illegal. npm lets you get away with it, but yarn v2 doesn't.

[AArnott]

You can read up on this yarn v2 distinct behavior in the link I put in the issue description.

[gr2m]

sounds like a problem with yarn 2 to me, maybe report the problem with them? I cannot fix one problem and introduce another.

There might be a more elegant solution, but I don't know what it would look like. I just use the types for convenience when working on this plugin, it has no impact whatsover to users of the plugin.

[AArnott]

I cannot fix one problem and introduce another.

What problem would you introduce?

sounds like a problem with yarn 2 to me, maybe report the problem with them?

They know about it. Did you read the link I shared? They allow for it by offering a "looseMode" which I've activated. But I agree with their take that this is a bug in your code.

Just imagine in abstract: I ship an NPM package with typings included that rely on types defined in another package. If I don't have a dependency expressed on that other package, anyone consuming my package would be unable to be consumed by a typescript user because typescript wouldn't be able to find the typings from the transitive package that wasn't added as a dependency. So this repros without yarn 2 and therefore isn't yarn 2's fault.
You've gotten away with this bug so far because you assumed your users also depend on this package so you were piggy backing off of them. But I hope you can see from my abstract argument that it's still a bug in your code that you don't express the dependency.

[AArnott]

I just found the problem you say would be introduced on the PR:

The problem is that if I declare @octokit/core as a production dependency, you can end up with multiple versions of it.

I am not an expert in npm version handling, but I thought that if you express your minimum version (e.g. ^3.0.0) and if your downstream consumer also expresses a version, and they are compatible that NPM/yarn would just select one matching version and share between the two. You only get two packages downloaded if they are in fact incompatible. At least, I think that's how it works.

And anyway FWIW, I do see many npm packages that do get brought in with multiple versions in an app and I've never seen it cause a problem.

[gr2m]

I'm glad the problem is not blocking for you.

I do plan to eventually set @octokit/core as a peer dependency on Octokit plugins, but it's not a priority right now.

If we added

  "peerDependencies": {
    "@octokit/core": ">=3"
  }

would that resolve the complaining from Yarn 2?

[AArnott]

would that resolve the complaining from Yarn 2?

I'm not sure. I suspect it would. I'm not sure how to test it in advance, but maybe I can find a forum to ask the yarn v2 folks.

[gr2m]

Usually every pull request has their own npm build, so you can install directly from every pull request, but there are some errors right now: https://github.com/octokit/plugin-paginate-rest.js/pull/128/checks?check_run_id=949586838, I'll look into that. Once that resolved you can send a PR to add the peerDependencies key and install it directly from that for testing

[gr2m]

using import type might also resolve the problem

import type only imports declarations to be used for type annotations and declarations. It always gets fully erased, so there’s no remnant of it at runtime

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html

[gr2m]

Can you please share the exact steps on how you run into this problem? I tried to reproduce it with Yarn 2, and had no problems

[AArnott]

Here is the minimal repo that repros the problem with the shipping npm package: https://github.com/AArnott/OctokitPaginateRest.Repro124

It has just a few commits: one for each step.

I'll now proceed to try testing your peerDependencies fix.

[AArnott]

@gr2m I tried to install your package from the PR as the instructions in the PR check prescribed, but it failed:

yarn add https://github.pika.dev/octokit/plugin-paginate-rest.js/pr/130
Internal Error: Invalid descriptor (https://github.pika.dev/octokit/plugin-paginate-rest.js/pr/130)
    at Module.R (/home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:2:346902)
    at /home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:2:23776
    at Array.map (<anonymous>)
    at M.execute (/home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:2:23664)
    at async M.validateAndExecute (/home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:2:551219)
    at async U.run (/home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:17:18859)
    at async U.runExit (/home/andrew/git/paginateRepro/.yarn/releases/yarn-berry.js:17:18993)

Any suggestions?

[gr2m]

Can you try again? I think there was an npm registry outage. It worked for me with the latest yarn

yarn add https://github.pika.dev/octokit/plugin-paginate-rest.js/pr/130
yarn add v1.22.4
info No lockfile found.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
warning " > @octokit/[email protected]" has unmet peer dependency "@octokit/core@>=3".
[4/4] 🔨  Building fresh packages...

success Saved lockfile.
success Saved 3 new dependencies.
info Direct dependencies
└─ @octokit/[email protected]
info All dependencies
├─ @octokit/[email protected]
├─ @octokit/[email protected]
└─ @types/[email protected]
✨  Done in 4.04s.

[gr2m]

Is https://github.com/AArnott/OctokitPaginateRest.Repro124 private?

[AArnott]

Oops. Yes it was private. I've fixed that. It's public now.
I'll retry your peer dependency fix soon.

[AArnott]

I still get the "Invalid descriptor" error. Now that my repo is public, can you try, @gr2m ?

[gr2m]

Thanks Andrew, I was able to reproduce the problem.

So I'm still not sure what to do about this 🤔 I do not appreciate yarn breaking with the way npm works.

For now, please use the "loose" mode. I'm not sure yet how I will address it, and I have other priorities I'm afraid

[AArnott]

Understandable. Thanks for looking into it.

[merceyz]

You can add this to .yarnrc.yml to apply the fix without having to use loose mode and/or grabbing the PR

packageExtensions:
  "@octokit/plugin-paginate-rest@*":
    peerDependencies:
      "@octokit/core": "*"

So I'm still not sure what to do about this

The fix is to declare it as a peerDependency, not just for PnP to work but for node_modules as well

I do not appreciate yarn breaking with the way npm works.

Breaking is the wrong term here, it's more like: yarn now lets you know when you're relying on undefined behaviour. For the current types to work you need the package manager to hoist @octokit/core to a level higher than or on the same level as @octokit/plugin-paginate-rest but if you don't declare that you need it there is no guarantee that will happen.

[AArnott]

@merceyz Thanks for the added info and the workaround. I'll try that right away.
It sounds like you agree that #130 will fix the issue then. That's awesome. :)

[gr2m]

Thank you both for your help. I'll add @octokit/core as a peerDependency. I'll do the same for the other plugins, too.

@AArnott Can you confirm that the problem is resolved with v2.3.1?

[github-actions]

🎉 This issue has been resolved in version 2.3.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[AArnott]

@gr2m I just tested, but it still repros. But it's reproing on v2.3.0 despite my adding 2.3.1 to my package.json file.
2.3.0 is being brought in by @actions/github by the looks of it from select portions of my yarn.lock file:

"@actions/github@npm:^4.0.0":
  version: 4.0.0
  resolution: "@actions/github@npm:4.0.0"
  dependencies:
    "@actions/http-client": ^1.0.8
    "@octokit/core": ^3.0.0
    "@octokit/plugin-paginate-rest": ^2.2.3
    "@octokit/plugin-rest-endpoint-methods": ^4.0.0
  checksum: 49d606acd5b74a2f3e41c76d5a5d54341943e43b79cb94639c6c01d228263a4a287ac43d02d6c872336ab02962d0a7904b1a821bb5d5966498a78c954e34c3f2
  languageName: node
  linkType: hard

"@octokit/plugin-paginate-rest@npm:^2.2.3":
  version: 2.3.0
  resolution: "@octokit/plugin-paginate-rest@npm:2.3.0"
  dependencies:
    "@octokit/types": ^5.2.0
  checksum: 3df28decd7af1c23fb1876ddc988f4c1a386febe1586a7b64d8c8334b3240de4ca4152074bfe83e8517c59d57a574fa3f39326c3ecf029a3f3411f08d576186a
  languageName: node
  linkType: hard

I guess @github/actions will need to be updated with the new version of plugin-paginate-rest for this to work out of the box.
In the meantime, is there a way to force yarn to resolve 2.3.1 from the ^2.2.3 request, @merceyz ?

[merceyz]

Yarn wont update that version since you didn't ask it to (and there is no way to ask it to either atm), so you'll have to remove the @octokit/plugin-paginate-rest entry from the lockfile to have yarn resolve it again to 2.3.1

[AArnott]

Very interesting, @merceyz. Thanks for all the tips. I was able to confirm the issue is fixed with 2.3.1.