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

fix: include type definitions when publishing #21

Merged
merged 17 commits into from
Oct 3, 2024
Merged

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Sep 27, 2024

Mainly:

  • Sets up type defs generation
  • Updates npm package setup to include generated types
  • BREAKING CHANGE: renames the exports to include the .js suffix at the end, which I believe is generally recommended/idiomatic. Not strictly necessary for us to do so open to reverting that change if preferred EDIT: reverted

Should introduce noUncheckedIndexedAccess in the tsconfig as a follow-up but it will lead to errors that will require code changes outside the scope of this PR.

lib/utils/streams.js Outdated Show resolved Hide resolved
@@ -80,6 +89,7 @@
"png-stream": "^1.0.5",
"prettier": "^3.3.3",
"random-bytes-readable-stream": "^3.0.0",
"rimraf": "^4.4.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Pinned to v4 since >= v5 removes support for node 18

package.json Show resolved Hide resolved
@achou11 achou11 marked this pull request as ready for review September 27, 2024 18:28
@achou11 achou11 requested a review from gmaclennan September 27, 2024 18:28
@achou11 achou11 changed the title fix: include type definitions when publishing fix!: include type definitions when publishing Sep 27, 2024
package.json Outdated Show resolved Hide resolved
@achou11 achou11 requested review from EvanHahn and gmaclennan and removed request for gmaclennan September 30, 2024 14:52
Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

great, thanks for this! Removing TransformOptions to satisfy typescript bothered me, so I fixed that with 31213d0

I don't agree with the change to add .js to exports. Most modules I have used that define exports do not include an extension, including node modules (e.g. fs/promises), and I think it leads to some confusion because it suggests a file is being imported. The file extension is required by ESM when importing a local file, but that is a different thing.

@achou11 achou11 changed the title fix!: include type definitions when publishing feat: include type definitions when publishing Oct 2, 2024
@achou11
Copy link
Member Author

achou11 commented Oct 2, 2024

Removing TransformOptions to satisfy typescript bothered me, so I fixed that with 31213d0

nice! I tried something almost similar to that before and it didn't work. glad you fixed it.

I don't agree with the change to add .js to exports. Most modules I have used that define exports do not include an extension, including node modules (e.g. fs/promises), and I think it leads to some confusion because it suggests a file is being imported. The file extension is required by ESM when importing a local file, but that is a different thing.

Yeah that makes sense. Reverted via 2df05b9

@achou11 achou11 requested a review from gmaclennan October 2, 2024 13:40
@achou11 achou11 removed the request for review from EvanHahn October 2, 2024 13:40
@achou11
Copy link
Member Author

achou11 commented Oct 2, 2024

One last thing to potentially resolve: what's the best way to expose the types exported from lib/types.ts? e.g. needing access to SMPStyle

EDIT 1: Hm looks like because of this, I may need to update this PR to emit source files too...

EDIT 2: Seems like I could get around this by adding another export, but not really sure if that's "proper":

    "./types": {
      "types": "./dist/types.d.ts"
    }

@gmaclennan
Copy link
Member

what's the best way to expose the types exported from lib/types.ts?

you can add this line to index.js:

/** @typedef {import('./types.js').SMPStyle} SMPStyle */

Then you can do:

import type { SMPStyle } from 'styled-map-package'

You could do the same in any of the other export entry points.

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

looks good to me!

package.json Show resolved Hide resolved
@achou11
Copy link
Member Author

achou11 commented Oct 3, 2024

what's the best way to expose the types exported from lib/types.ts?

you can add this line to index.js:

/** @typedef {import('./types.js').SMPStyle} SMPStyle */

Then you can do:

import type { SMPStyle } from 'styled-map-package'

You could do the same in any of the other export entry points.

Awesome, seems to work! (see 05084f9)

@achou11 achou11 changed the title feat: include type definitions when publishing fix: include type definitions when publishing Oct 3, 2024
@achou11 achou11 merged commit 1997ed1 into main Oct 3, 2024
3 checks passed
@achou11 achou11 deleted the ac/type-fixes branch October 3, 2024 20:39
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