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

TypeScript declaration is missing #50

Closed
ukstv opened this issue Jul 14, 2022 · 4 comments · Fixed by #53
Closed

TypeScript declaration is missing #50

ukstv opened this issue Jul 14, 2022 · 4 comments · Fixed by #53
Labels
good first issue Good issue for new contributors need/analysis Needs further analysis before proceeding

Comments

@ukstv
Copy link
Contributor

ukstv commented Jul 14, 2022

package.json states that typescript declaration file should be in ./dist/src/types.d.ts. Apparently, there is no dist folder in the first place. What is the best course of action to add TS types here?

@ukstv ukstv added the need/triage Needs initial labeling and prioritization label Jul 14, 2022
@RangerMauve
Copy link

Adding a d.ts in the root will likely be enough.

@achingbrain
Copy link
Member

The "types" field in package.json looks like a mistake since this module doesn't have types.

We can either:

  1. Add typedoc to ./src/index.js and generate types emitDeclarationOnly-style as part of the build
  2. Hand-roll a .d.ts file as @RangerMauve suggests

Then update the "types" field to point at wherever the file ends up.

My preference would be 1. as errors tend to creep into hand-rolled .d.ts files and it's not straightforward to lint/otherwise check them, but 2. will be quicker and this is a very simple module.

@ukstv
Copy link
Contributor Author

ukstv commented Aug 4, 2022

It seems, that types are generated now. npm pack adds dist folder to the package even. Though, a published artefact on NPM does not have it.

IMO, the only thing needed here is to change types to dist/src/index.d.ts and make sure that a publishing workflow does not mess up with what is stated in package.json.

What is the reason, that a generated dist folder is not included into the package?

@achingbrain achingbrain added good first issue Good issue for new contributors need/analysis Needs further analysis before proceeding and removed need/triage Needs initial labeling and prioritization labels Oct 14, 2022
@SgtPooki SgtPooki self-assigned this Oct 14, 2022
@SgtPooki
Copy link
Member

Looks like the CI action doesn't run npm build or anything: https://github.com/ipfs/npm-go-ipfs/blob/ce5751157af497066a3913d58bb9b682bc93c671/.github/actions/publish/entrypoint.sh#L34-L63

╰─ ✔ ❯ git log -1 | Cat
commit ce5751157af497066a3913d58bb9b682bc93c671
Author: galargh <galargh@users.noreply.github.com>
Date:   Tue Oct 4 11:10:30 2022 +0000

    0.16.0
    
╰─ ✘ 1 ❯ npm pack # No npm install
npm notice
npm notice 📦  go-ipfs@0.16.0
npm notice === Tarball Contents ===
npm notice 1.1kB LICENSE
npm notice 4.5kB README.md
npm notice 59B   bin/ipfs
npm notice 1.5kB package.json
npm notice 7.2kB src/download.js
npm notice 472B  src/go-platform.js
npm notice 471B  src/index.js
npm notice 131B  src/post-install.js
npm notice 1.2kB test/download.js
npm notice 295B  test/fixtures/clean.js
npm notice 184B  test/fixtures/example-project/package.json
npm notice 1.5kB test/install.js
npm notice 575B  test/path.js
npm notice 947B  tsconfig.json
npm notice === Tarball Details ===
npm notice name:          go-ipfs
npm notice version:       0.16.0
npm notice filename:      go-ipfs-0.16.0.tgz
npm notice package size:  7.4 kB
npm notice unpacked size: 20.1 kB
npm notice shasum:        6d1b569d17fda85baa4ff589d2c36cb2d0cb486b
npm notice integrity:     sha512-yFOz/+6/EsNba[...]NYpzd7TRXleCg==
npm notice total files:   14
npm notice
go-ipfs-0.16.0.tgz

But if we run npm install, and then prepublishOnly, (called by npm publish) pack includes the right things.

╰─ ✔ ❯ npm run prepublishOnly

> go-ipfs@0.16.0 prepublishOnly
> tsc


╰─ ✔ ❯ npm pack
npm notice
npm notice 📦  go-ipfs@0.16.0
npm notice === Tarball Contents ===
npm notice 1.1kB  LICENSE
npm notice 4.5kB  README.md
npm notice 1.7kB  dist/package.json
npm notice 185B   dist/src/download.d.ts
npm notice 7.5kB  dist/src/download.js
npm notice 56B    dist/src/go-platform.d.ts
npm notice 550B   dist/src/go-platform.js
npm notice 32B    dist/src/index.d.ts
npm notice 506B   dist/src/index.js
npm notice 11B    dist/src/post-install.d.ts
npm notice 134B   dist/src/post-install.js
npm notice 25.7kB dist/tsconfig.tsbuildinfo
npm notice 1.5kB  package.json
npm notice 7.2kB  src/download.js
npm notice 472B   src/go-platform.js
npm notice 471B   src/index.js
npm notice 131B   src/post-install.js
npm notice 1.2kB  test/download.js
npm notice 295B   test/fixtures/clean.js
npm notice 184B   test/fixtures/example-project/package.json
npm notice 1.5kB  test/install.js
npm notice 575B   test/path.js
npm notice 947B   tsconfig.json
npm notice === Tarball Details ===
npm notice name:          go-ipfs
npm notice version:       0.16.0
npm notice filename:      go-ipfs-0.16.0.tgz
npm notice package size:  18.1 kB
npm notice unpacked size: 56.4 kB
npm notice shasum:        2cc638c554f4c266afb3a0508b737f07c03f2f12
npm notice integrity:     sha512-almd/aceMAyi9[...]V3RrqjzogmK3w==
npm notice total files:   23
npm notice
go-ipfs-0.16.0.tgz

@achingbrain it looks like we just need to include an npm install in the CI that publishes the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for new contributors need/analysis Needs further analysis before proceeding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants