-
Notifications
You must be signed in to change notification settings - Fork 199
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
feat: Expose TypeScript declarations #225
Conversation
src/index.js
Outdated
/** | ||
* @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({ |
There was a problem hiding this comment.
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
@argyleink Regarding the TypeScript issue in 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
I've tried various alterations to the const { OpenProps } = require('open-props') import { OpenProps } from 'open-props' I validated this update by:
IMPORTANT: This would be a breaking change!Hopefully, someone more familiar with |
There was a problem hiding this 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?
src/index.js
Outdated
/** | ||
* @template T | ||
* @param {T} props | ||
* @returns {import("type-fest").CamelCasedPropertiesDeep<T>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @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 😃
There was a problem hiding this comment.
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
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
That's what I figured, makes sense 👍 Might just have to introduce the |
Here's a repo I used to test autocomplete and imports for this PR: cf. #223 (comment) |
@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! |
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 |
available in |
Do not merge: See
TODO
section belowThis 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:tsconfig.json
defining thedist/types
output directorypackage.json
"types":
and"exports.types":
keys to resolve the emitteddist/types/index.d.ts
any
types in thesrc/index.js
default exportJSDoc
annotation to helptsc
understand the camel case keys transform on the default exportAfter running
npm run lib:js
Example
npm link
ing and using the intellisense in a.mjs
fileImportant
There is still one issue I haven't resolved and that is that the declarations in
![image](https://user-images.githubusercontent.com/32409546/163602503-706ea08c-da45-4edb-8ff3-61682a264e8d.png)
.cjs
modules want to resolve values offOpenProps.default
:TODO:
.cjs
files think props are accessible fromOpenProps.default
tsconfig
compilerOptions
and ensure the appropriate defaults have been set