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

[BUG]: dist-types/index.d.ts is not present in the published NPM package #405

Closed
1 task done
maxnachlinger opened this issue Feb 20, 2023 · 21 comments · Fixed by #410
Closed
1 task done

[BUG]: dist-types/index.d.ts is not present in the published NPM package #405

maxnachlinger opened this issue Feb 20, 2023 · 21 comments · Fixed by #410
Assignees
Labels
released Type: Bug Something isn't working as documented

Comments

@maxnachlinger
Copy link

What happened?

Hey awesome maintainers! After upgrading from 4.1.1 to 4.1.2 I noticed the following tsc error:

 error TS7016: Could not find a declaration file for module '@octokit/plugin-retry'. '/some-path-here/node_modules/.pnpm/@octokit+plugin-retry@4.1.2_@octokit+core@4.2.0/node_modules/@octokit/plugin-retry/dist-node/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/octokit__plugin-retry` if it exists or add a new declaration (.d.ts) file containing `declare module '@octokit/plugin-retry';`

2 import { retry } from '@octokit/plugin-retry';

Having a look in the tgz on npm I only see a dist-node directory, but the package.json exports the types in dist-types/index.d.ts

Versions

"@octokit/core": "4.2.0"
"@octokit/plugin-retry": "4.1.2",
"@octokit/rest": "19.0.7",
node 16.19.1

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@maxnachlinger maxnachlinger added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Feb 20, 2023
@oscard0m
Copy link
Member

I started checking this:

Here are two logs comparing a release with types and without types:

Release 4.1.1

@pika/pack build v0.3.7
[1/6] Validating source...
[2/6] Preparing pipeline...
      ❇️  pkg/
      » tsconfig.json [compilerOptions.target] should be "ES2020", but found "ES2018". You may encounter problems building.
[3/6] Running @pika/plugin-ts-standard-pkg...
      📝  pkg/dist-src/index.js [esnext]
      📝  pkg/dist-types/index.d.ts [types]
      » Linting with standard-pkg...
[4/6] Running @pika/plugin-build-node...
      📝  pkg/dist-node/index.js [main]
[5/6] Running @pika/plugin-build-web...
      📝  pkg/dist-web/index.js [module]
[6/6] Finalizing package...
      » copying LICENSE...
      » copying README.md...
      📝  pkg/package.json
      📦  pkg/
npm notice 📦  @octokit/plugin-retry@4.1.1
npm notice === Tarball Contents === 
npm notice 1.1kB LICENSE                      
npm notice 2.5kB README.md                    
npm notice 2.9kB dist-node/index.js           
npm notice 6.2kB dist-node/index.js.map       
npm notice 677B  dist-src/error-request.js    
npm notice 921B  dist-src/index.js            
npm notice 32B   dist-src/version.js          
npm notice 1.4kB dist-src/wrap-request.js     
npm notice 105B  dist-types/error-request.d.ts
npm notice 401B  dist-types/index.d.ts        
npm notice 40B   dist-types/version.d.ts      
npm notice 91B   dist-types/wrap-request.d.ts 
npm notice 2.9kB dist-web/index.js            
npm notice 6.0kB dist-web/index.js.map        
npm notice 1.2kB package.json

Release 4.1.2

@pika/pack build v0.3.7
[1/6] Validating source...
[2/6] Preparing pipeline...
      ❇️  pkg/
      » tsconfig.json [compilerOptions.target] should be "ES2020", but found "ES2018". You may encounter problems building.
[3/6] Running @pika/plugin-ts-standard-pkg...
      📝  pkg/dist-src/index.js [esnext]
      📝  pkg/dist-types/index.d.ts [types]
      » Linting with standard-pkg...
[4/6] Running @pika/plugin-build-node...
      📝  pkg/dist-node/index.js [main]
[5/6] Running @pika/plugin-build-web...
      📝  pkg/dist-web/index.js [module]
[6/6] Finalizing package...
      » copying LICENSE...
      » copying README.md...
      📝  pkg/package.json
      📦  pkg/
npm notice 📦  @octokit/plugin-retry@4.1.2
npm notice === Tarball Contents === 
npm notice 1.1kB LICENSE           
npm notice 2.5kB README.md         
npm notice 2.9kB dist-node/index.js
npm notice 1.2kB package.json   

Tagging @octokit/js-community @octokit/js just in case somebody knows where the issue could be while I check it.

@oscard0m oscard0m self-assigned this Feb 20, 2023
@oscard0m
Copy link
Member

oscard0m commented Feb 20, 2023

Ok, so... the root cause of this issue is the following one, I think:

Apparently, since npm 9 the file pattern matching for npm publish has changed, but nothing was reported in their Changelog (I'm assuming it's an unintended change).

The generated package.json under pkg/ folder after running pika-pack build contains the following:

"files": [
    "dist-*/",
    "bin/"
  ],

The fix would be as easy as adding ** to the file patterns until npm gives a solution to this:

"files": [
    "dist-*/**",
    "bin/**"
  ],

Since these lines are auto-generated and pika-pack is unmaintained, I guess we need to decide what to do here.

I guess the quick solution is to use npm@8 for semantic-release until we find a proper solution for everything.

What do you think @octokit/extensibility-sdks @octokit/js-community?

@kfcampbell
Copy link
Member

@oscard0m thank you for the investigation here! That's fascinating. I know npm 9 was released in October; I wonder why this is just happening now. Perhaps it's related to a shift in the LTS version in our node setup?

Pinning down a date would help with the solution I think: if we started using npm 9 to release immediately before the issue was reported, a rollback seems appropriate. If we've been using npm 9 since October, rolling back a major version may have additional complications to think about.

@wolfy1339
Copy link
Member

We used to explicitly define a requirement on semantic-release in our devDependencies, until a new version of semantic-release broke compatibility with a version of node we were supporting at the time, and I believe that version introduced npm@9

Would we be able to patch the files afterwards pika has built the package before semantic-release publishes a release?

@oscard0m
Copy link
Member

oscard0m commented Mar 2, 2023

@kfcampbell opened a PR with @wolfy1339 's proposal as a hot fix (with some question marks) and opened a discussion to talk about a strategy for a long-term solution: octokit/octokit.js#2403

oscard0m added a commit that referenced this issue Mar 2, 2023
oscard0m added a commit that referenced this issue Mar 2, 2023
@wolfy1339 wolfy1339 added Priority: High and removed Status: Triage This is being looked at and prioritized labels Mar 3, 2023
@wolfy1339 wolfy1339 moved this from 🆕 Triage to 🏗 In progress in 🧰 Octokit Active Mar 3, 2023
@Ddupasquier
Copy link

Thanks for showing me to this issue @wolfy1339. I patiently await updates.

wolfy1339 pushed a commit that referenced this issue Mar 3, 2023
* fix(ci): add hotfix in built package.json to use proper file patterns in "files"

You can read more here: #405

* ci(release): run 'scripts/fix-package-json.js' before release
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in 🧰 Octokit Active Mar 3, 2023
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

🎉 This issue has been resolved in version 4.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Ddupasquier

This comment was marked as off-topic.

@wolfy1339

This comment was marked as off-topic.

@Ddupasquier

This comment was marked as off-topic.

@wolfy1339

This comment was marked as off-topic.

@Ddupasquier

This comment was marked as off-topic.

@oscard0m oscard0m closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2023
@oscard0m oscard0m closed this as completed Mar 5, 2023
@oscard0m
Copy link
Member

oscard0m commented Mar 5, 2023

Here's a comparison of exported files from 4.1.2 and 4.1.3:

Published files in 4.1.2:

image

Published files in 4.1.3:

image

@Ddupasquier
Copy link

Omg, if this is actually done I will buy everyone involved a beer.

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

@maxnachlinger @Ddupasquier did you have a chance to try this? Did your problems get solved?

@maxnachlinger
Copy link
Author

@oscard0m forgive me for not circling back. Your dazzling fix solved the issue! :)

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

@oscard0m forgive me for not circling back. Your dazzling fix solved the issue! :)

No problem! I was just going through my old notifications and I just wanted to double-check everything was working again.

I'm happy we unblocked you @maxnachlinger :)

@Ddupasquier
Copy link

@oscard0m In order to give you an accurate answer I will have to revisit the component and remember what I was even doing haha. On first view of the code... No. But this could be a syntax issue on my part or maybe my token expired.
I'll get back to you when I know more.

@Ddupasquier
Copy link

Ddupasquier commented Apr 7, 2023

@oscard0m AH! So if you check the 'unrelated comments' a little ways up between @wolfy1339 and I, it seems that the issue from the octokit side is solved.

#405 (comment)

But success in my project is still blocked haha.
Wolfy tipped me off to where octokit is tracking this issue as well:
octokit/core.js#547

@oscard0m
Copy link
Member

oscard0m commented Apr 7, 2023

Yep, the @octokit-next is blocking quite a few things right now... I'm sorry to hear that. But I assume this issue can remain closed.

@Ddupasquier
Copy link

Absolutely. I appreciate everyone's dillogence!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants