-
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
add TS support #334
add TS support #334
Conversation
|
nice! thanks for contributing types 🙂 i wonder if switching the repo from node to deno would be a good option. ts would be a first class citizen for within the lib and for those importing it 🤔 |
I believe this is ready for review now. What would be the best way to test it? As for deno support, I think it would be nice but for node users there would still need to be |
"gen:shadowdom": "cd build && node props \"\" false \":host\" \"shadow\"", | ||
"gen:prefixed": "cd build && node props.js \"op\" true", | ||
"module:js": "npm run bundle:pre-edit && microbundle --no-sourcemap && npm run bundle:post-edit", | ||
"bundle:fix-dts-name": "cd dist && node -e \"require('fs').renameSync('index.d.ts', 'open-props.d.ts')\"", |
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.
this part is a bit weird. microbundle names the types files index.d.ts
even though the "types"
field above says open-props.d.ts
. so i'm just renaming it 😄
lookin pretty good! i gave it a spin with some imports like this: import OP from '../src/index'
import {Indigo} from '../src/props.colors'
import Sizes from '../src/props.sizes'
import Gradients from '../src/props.gradients'
// OP ✅
// Indigo ✅
// Sizes ✅
// Gradients ✅ While I appreciated the minimal touch & generated strategy from #225, this tested well for me locally, doesnt seem to have affected modules and is ready now. Do @hchiam or @aaronccasanova wanna add a review / QA / thoughts? |
that's awesome. if you could make a snapshot/dev release from this branch, i can test it on my end as well. |
Could give this a try? |
that wouldn't have the output from build/bundle scripts. i think it would be better to test the artifact exactly as it would be when published to npm |
I'm not sure how to make a release from a PR? I'd have to merge it first yeah? |
hmm, for releasing from a branch, i would do this:
|
gotcha, so i'd be manually submitting a tagged version to NPM. there's gotta be a better way to test it?! which, btw, the final bundle already contains src, no reason to avoid that? folks reach into those files all the time, whether it's for postcss or js. the final bundle just has a flattened architecture for easy file access on unpkg or npm import, and then the built |
i can try the npm link test soon too 👍🏻 |
ah ok, i didn't know the published output contains the whole source. that would mean the as i was trying to read up on findings: |
"target": "es2022", | ||
"module": "es2022", |
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.
not in this pr, but maybe in future, we should investigate es5 for wider a11y? i'm aware of the ie sunset but i still deal with clients who have users on ie
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.
i think further transformations could be done through the bundler? unless you're trying to import directly from a CDN
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.
code so far LGTM! 👍
i think this is because of the export maps (i am running in an ESM environment). Line 33 in 29e8f2b
but because we added a wildcard recently, it can be worked around. Line 173 in 29e8f2b
After some more testing: // Node ESM works. TS fails.
import { Indigo } from 'open-props/src/colors';
// Node ESM fails. TS works with moduleResolution: "Node".
import { Indigo } from 'open-props/src/props.colors';
// Node ESM and TS are both happy and work
import { Indigo } from 'open-props/src/props.colors.js'; To correctly support TS from |
Thanks, yeah I tried my best to avoid a full project conversion to TypeScript and leverage the built-in Microbundle mechanics. Looks like this PR is generating declaration files with
Sorry folks, I don't have much bandwidth! That said, if you do want the source to stay pure ESM, there may still be more opportunities to explore using |
sorry i havent gotten to this yet!
agree, but also… it seems crazy that its taking all this work to get TS to autocomplete a static object. like, why doesnt this just work out of the box and it can introspect to the default export and infer typeahead values. there's gotta be a better way? its not like each value needs typed for users to feel empowered, they just want autocomplete for property names. so invasive to this code to just get that one little feature for folks. TS being very greedy and needy here, I don't like it. |
well, what would you like me to do?
and of course there is still the issue of Node ESM + types i mentioned above. |
i was mostly abstractly whining as opposed to asking for changes. this conceptually should be easier i feel is all, bummer to see 3 separate PRs all struggle in different ways to type an export file. feels like this repo is in a hard spot without an easy win; there's so many tradeoffs when what we expect is just to add some features:
it's smelly (to me at least) when 1 little feature (type ahead on a flat key/value object) has to cover so many files and flows with it's process in order to deliver that DX. i dont want to break JS users workflows because i wanted to add TS user workflows, that's not a fair tradeoff. both types of users should be happy consuming these variables. they're just variables! this should just be an addition for TS, not a TS conversion exercise right? maybe microbundle is part of the problem too here? the bundled output is using object.assign(), probably making it hard for TS to infer types and properties. If that was simpler, would TS "just pick it up"? maybe we look at #225 again and remove microbundle? i'm sorry for complicating things / not just hitting merge yet. i just feel like there's gotta be a better way to get typeahead for TS users.. it's not like we even want types, we just want typeahead. the types are all strings, maybe a number here or there. all the consuming project wants is auto complete without a headache. |
what if we remove microbundle, cuz who cares if the dist directly has a minified and obfuscated object in it (authors will bundle), so that the file authors import is a super simple structured object that types can easily be inferred from? then we dont deal with any TS in the project and authors get autocomplete? Imagine something like this was in const openprops = {
'--size-1': '1rem',
size1: '1rem',
'--size-2': '2rem',
size2: '2rem',
// etc
} simplifies the library, no effects to current JS users, and TS gets a more straightforward object it can deal with better? |
makes sense to me. microbundle is the reason i had to use
to avoid breaking changes, we'll probably need to make sure the same files with same names continue to exist in |
closes #333
.js
to.ts
.js
/.d.ts
usingtsc
(currently blocked. see #334 (comment))