add TS support

[mayank99]

closes #333

• converted all relevant files from .js to .ts
• compiling those files into .js/.d.ts using tsc

(currently blocked. see #334 (comment))

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[argyleink]

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 🤔

[mayank99]

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 .js and .d.ts files.

[mayank99]

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 😄

[argyleink]

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?

[mayank99]

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 ✅

that's awesome. if you could make a snapshot/dev release from this branch, i can test it on my end as well.

[argyleink]

Could give this a try? "open-props": "mayank99/open-props"?

[mayank99]

"open-props": "mayank99/open-props"?

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

[argyleink]

I'm not sure how to make a release from a PR? I'd have to merge it first yeah?
What about NPM link?
Def agree we should test on the bundled results before merging to main, I only tested the source.

[mayank99]

hmm, npm link would have the same problem of containing the entire source code instead of just the final bundle.

for releasing from a branch, i would do this:

1. change "version" in package.json to something like 1.5.3-dev.0
2. run npm run build and npm run bundle
3. run npm publish --tag dev.
   • the "tag" here is important so you don't push to latest
   • this assumes you are logged into npm or have your token connected

[argyleink]

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 dist/ directory with JS modules. this should be totally g2g to test locally with npm link yeah? run the build commands inside your NPM linked project and then point to it from a test project, and you got yourself a very high fidelity local dev env for testing this out, all without deploying a dummy tag to npm.

[argyleink]

i can try the npm link test soon too 👍🏻

[mayank99]

ah ok, i didn't know the published output contains the whole source. that would mean the .ts files also get included. maybe we should npmignore those? (also i noticed when running npm run build and npm run bundle that hundreds of css files get added to the repo. maybe those need to be gitignored?)

as i was trying to read up on npm link, i actually found a more reliable way to test it: npm pack. so i ran that, it created a .tgz file identical to what would be published on npm. then i ran npm install ./<path-to-generated-file>.tgz in a different project to test.

findings:

• the default export works well: import OP from 'open-props'
• the default export in src/index also works well: import OPSrc from 'open-props/src'
• anything else inside src/ errors and doesn't have autocomplete
  

[hchiam]

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

[mayank99]

i think further transformations could be done through the bundler? unless you're trying to import directly from a CDN

[hchiam]

code so far LGTM! 👍

[mayank99]

• anything else inside src/ errors and doesn't have autocomplete

i think this is because of the export maps (i am running in an ESM environment).

open-props/package.json

Line 33 in 29e8f2b

"./src/colors": "./src/props.colors.js",

but because we added a wildcard recently, it can be worked around.

open-props/package.json

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 'open-props/src/colors', we would need to add "types" field to every export, and ask users to use moduleResolution: "NodeNext". See docs: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing

[aaronccasanova]

While I appreciated the minimal touch & generated strategy from #225, [...]

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 tsc, which is where I was headed here, so I hope you have better luck with the ESM/CJS interop issues around default export types.

Do @hchiam or @aaronccasanova wanna add a review / QA / thoughts?

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 tsc with some combination of flags (e.g. --emitDeclarationOnly, --allowJs, etc) to emit declarations from the .js files. Perhaps @mayank99 already tried that! Otherwise, this PRs conversion to .ts source files seems like the next logical step 👍

[argyleink]

sorry i havent gotten to this yet!

conversion to .ts source files seems like the next logical step

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.

[mayank99]

well, what would you like me to do?

i might be able to undo some of the JS->TS conversions in build/ directory if i make it so .js files are generated before those scripts are run. (edit: done)
but i was not able to get everything to work properly with only JS files in the src/ directory.

and of course there is still the issue of Node ESM + types i mentioned above.

[argyleink]

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:

• changing all js files to ts files, typing them, and bundling them.. would still need .js files for users who arent coming from ts. import { Indigo } from 'open-props/src/colors.js' would need to exist, so then this repo would need to have both files in source? or duplicate all the files into a import { Indigo } from 'open-props/ts/colors' folder? so much duplication.. we could have TS generate the JS files at build time for the npm package..? jeez.
• feat: Expose TypeScript declarations #225 still feels like the right way forward.. just provide types for the export. JS's gnarly module world is adding unnecessary complication here. I could see myself making the tradeoff that CJS users of the lib just need to deal with the .export property and we can move on.
• this doesnt feel like a good way forward / thing to ask users to do: add "types" field to every export, and ask users to use moduleResolution: "NodeNext". but maybe it's fine? i'd be pretty frustrated if i started a new ts project and found the modules werent importing and scoured docs to find a tiny flag to set just so they resolve.. that's stealing DX for far more time than them just looking up a variable name.

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.

[argyleink]

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 dist/ instead of the current Object.assign() monstrosity currently deployed?

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?

[mayank99]

what if we remove microbundle

makes sense to me. microbundle is the reason i had to use .ts files in src (instead of .js with allowJs).

no effects to current JS users

to avoid breaking changes, we'll probably need to make sure the same files with same names continue to exist in dist/, even if they are not produced by microbundle.