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: Expose TypeScript declarations #225

Conversation

aaronccasanova
Copy link
Contributor

@aaronccasanova aaronccasanova commented Apr 15, 2022

Do not merge: See TODO section below

This PR updates the lib:js build to additionally output TypeScript declarations. Microbundle already handles TypeScript projects with zero config and thus only a few tweaks were needed to emit declaration files:

  • Added a tsconfig.json defining the dist/types output directory
  • Updated the package.json "types": and "exports.types": keys to resolve the emitted dist/types/index.d.ts
  • Added the type-fest (TypeScript utility types package) as a dev dependency to address the any types in the src/index.js default export
  • Added a JSDoc annotation to help tsc understand the camel case keys transform on the default export

After running npm run lib:js

image

Example npm linking and using the intellisense in a .mjs file

image

Important

There is still one issue I haven't resolved and that is that the declarations in .cjs modules want to resolve values off OpenProps.default:
image

TODO:

  • - Debug why .cjs files think props are accessible from OpenProps.default
  • - Take a second pass at the tsconfig compilerOptions and ensure the appropriate defaults have been set

src/index.js Outdated
Comment on lines 19 to 29
/**
* @template T
* @param {T} props
* @returns {import("type-fest").CamelCasedPropertiesDeep<T>}
*/
const keysToCamelCase = (props) => {
for (var prop in props) props[camelCase(prop)] = props[prop]
return props
}

const OpenProps = mapToObjectNotation({
const OpenProps = keysToCamelCase({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen.Recording.2022-04-15.at.10.26.26.AM.mov

@aaronccasanova
Copy link
Contributor Author

aaronccasanova commented Apr 16, 2022

@argyleink Regarding the TypeScript issue in CommonJs modules. As shown below, TypeScript currently thinks tokens are accessible from OpenProps.default[token] (on the default export). Whereas, they are actually accessible directly from OpenProps (e.g. OpenProps.animationBlink):

image

I'm struggling to identify the exact issue, but have narrowed things down a bit. Microbundle by default sets the rollup.OutputOptions.exports to 'auto' which according to the output.exports section of the Rollup docs could be a factor if default option is inferred:

default – if you are only exporting one thing using export default ...; note that this can cause issues when generating CommonJS output that is meant to be interchangeable with ESM output, see below

I've tried various alterations to the Microbundle setup and tsconfig and only managed to get predictable results by switching src/index.js from a default export to a named export: e.g.

const { OpenProps } = require('open-props')
import { OpenProps } from 'open-props'

I validated this update by:

  • npm linking open-props to a local test-open-props-types project
  • Creating a .mjs and .cjs file that named import OpenProps and log OpenProps.animationBlink
  • Ran both scripts verifying the logs
  • Ran a type check with the minimum compilerOptions npx tsc <file> --noEmit --allowJs --checkJs

IMPORTANT: This would be a breaking change!

Hopefully, someone more familiar with microbundle and hybrid ESM/CommonJs packages can chime in to avoid introducing a breaking change. However, as mentioned above converting to named exports was the only success I've had.

@argyleink argyleink mentioned this pull request Apr 18, 2022
Copy link
Owner

@argyleink argyleink left a comment

Choose a reason for hiding this comment

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

nice, leaning on microbundle is a good strategy here 👍🏻

during testing I couldnt figure out why/how the typed export is missing all the longform string accessors like op["--size-1"] as only op.size1 works. looking at the generated index.d.ts I can see that the string versions are missing. the JS api should support both object and array accessor styles, and I wasnt able to find a swift resolution in 15m. thoughts on this?

Regarding default on the CJS export, this has been an area of frustration, and I've done lots of iterating to get it to the current state. I'd prefer to not create any breaking changes, let's continue iterating here to see what we can do.

there's another engineer working to add types in a different fashion, maybe y'all can work together and find the best way to provide types without requiring changes to the current output?

hchiam added a commit to hchiam-test-org/svelte-kit-typescript-open-props2 that referenced this pull request Apr 18, 2022
src/index.js Outdated
/**
* @template T
* @param {T} props
* @returns {import("type-fest").CamelCasedPropertiesDeep<T>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {import("type-fest").CamelCasedPropertiesDeep<T>}
* @returns {import("type-fest").CamelCasedPropertiesDeep<T> & import("type-fest").KebabCase<T>}

to get both op.size1 and op["--size-1"] to work, to address part of @argyleink's feedback #225 (review)

I'll keep investigating my alternative solution, but I'm also liking the approach in this PR 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! T already infers the kebab keys from the original type, so I just needed to intersect T with the camel cased type: bd34e2f

@aaronccasanova
Copy link
Contributor Author

aaronccasanova commented Apr 19, 2022

the JS api should support both object and array accessor styles, and I wasnt able to find a swift resolution in 15m. thoughts on this?

It's a quick update bd34e2f. For some reason I thought the original utility was replacing the dashed properties with camel case keys. The fix is simply intersecting the original inferred type T. I should probably rename keysToCamelCase to addCamelCasedProperties or leave the original mapToObjectNotation name.

image

I'd prefer to not create any breaking changes, let's continue iterating here to see what we can do.

That's what I figured, makes sense 👍 Might just have to introduce the typescript dev dependency and run tsc with the --emitDeclarationsOnly option. I'll play around with microbundle a bit more and otherwise fallback to tsc.

@hchiam
Copy link
Contributor

hchiam commented May 14, 2022

Here's a repo I used to test autocomplete and imports for this PR:

https://github.com/hchiam-test-org/svelte-kit-typescript-open-props2/blob/main/my-app/package.json#L37 (WIP)

cf. #223 (comment)

@aaronccasanova
Copy link
Contributor Author

aaronccasanova commented Aug 13, 2022

@argyleink I haven't had the bandwidth to debug the ESM/CJS interop issues. Feel free to close the PR and/or @hchiam you're welcome to take it over!

@Jarvis1010
Copy link

does this still need help? I would be happy to pick it up since I would love to get the Types working as first-class citizens

@mayank99 mayank99 mentioned this pull request Jan 13, 2023
@argyleink argyleink mentioned this pull request Jan 17, 2023
@argyleink
Copy link
Owner

available in v1.5.4!

@argyleink argyleink closed this Jan 24, 2023
@aaronccasanova aaronccasanova deleted the feat/typescript-declarations branch January 24, 2023 06:16
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.

4 participants