react-strict-dom icon indicating copy to clipboard operation
react-strict-dom copied to clipboard

chore: Update build script to generate flow and TS types:

Open nmn opened this issue 1 year ago • 11 comments

#Fixes #25

Using StyleX as a starting point, overhauling the build scripts.

  • [x] Copy the gen-flow wrapper around flow-api-translator
  • [x] Set up script to generate per-file Flow and TS type-defs
  • [x] Update the package.json to point to the new output files
  • [x] Delete the existing build steps

Skipped these steps as we decided to keep flat bundles for now:

  • [ ] ~~Set up a Babel translation step for publishing to NPM that doesn't pre-bundle~~
  • [ ] ~~Set up a Babel translation step that translates to Haste modules~~

nmn avatar Feb 24 '24 05:02 nmn

compressed-size: runtime library

Size change: +15.90 kB 🔴 Total size: 15.90 kB

Filename: gzip (minify) kB size kB change % change
./packages/react-strict-dom/dist/dom/index.js 2.90 (8.66) +2.90 (+8.66) +100.0% (+100.0%) 🔴
./packages/react-strict-dom/dist/dom/runtime.js 0.92 (2.26) +0.92 (+2.26) +100.0% (+100.0%) 🔴
./packages/react-strict-dom/dist/native/index.js 12.08 (37.79) +12.08 (+37.79) +100.0% (+100.0%) 🔴

github-actions[bot] avatar Feb 24 '24 05:02 github-actions[bot]

Nice! I kind of want to keep flat bundling for OSS. And I wish this wasn't so much more complicated than the current setup (apart from existing flow lib def script). This isn't a modular library and we've previously found benefits in size and app bundling perf from pre-bundling libraries like react. Plus, to retain the most useful perf CI data we need flat bundles. AFAICT the motivation for this change is because flow doesn't support automatically generating lib defs

necolas avatar Feb 24 '24 07:02 necolas

I kind of want to keep flat bundling for OSS

One downside of flat bundling is that you can take advantage of tree-shaking in modern bundlers.

However, if we stop here, we can keep the flat bundles for the JS code and only keep the separate files for the type-defs.

This isn't a modular library

If we only publish an ESM build to NPM, we can limit what we export so that all the other files are "private".

AFAICT the motivation for this change is because flow doesn't support automatically generating lib defs

The flow-api-translater package being used by the script is the Flow teams solution for generating lib-defs. The other motivation is that this lets us generate TS type-defs as well. A folder of file like this is fully supported in both Flow and TS and so we don't really gain anything by bundling them into flat files.

nmn avatar Feb 26 '24 00:02 nmn

There's nothing to tree-shake out of this package on web, or even native really. AFAIK, tree-shaking / DCE isn't limited to what's in different files and should work if exports within a file aren't used. So we can focus on the types side of things

necolas avatar Feb 26 '24 02:02 necolas

@necolas That makes things simple enough. This PR is now ready.

I simply changed the output path from dist/dom.js to dist/dom/index.js and not the separate type files work as expected. The dist/dom/index.d.ts and dist/dom/index.js.flow files shadow the actual file and that works for both TS and Flow.

I can probably make this set up work for internal sync as well.

nmn avatar Feb 26 '24 04:02 nmn

Update: Made a couple of changes to ensure that no two files in the package have the same name. (Except when there are dom and native counter-parts`). This was mostly true already except:

  1. There was one unused file
  2. There are two versions of flattenStyle.js that work similarly but with different types.

For now, I've renamed one of the flattenStyle.js to flattenStyleXStyles.js, but eventually, it should be deleted.

This work has been done to be compatible with Haste for internal sync.

nmn avatar Feb 26 '24 06:02 nmn

Not able to reproduce this failure locally. I don't even have the file that is erroring. :(

nmn avatar Feb 27 '24 07:02 nmn

Not able to reproduce this failure locally. I don't even have the file that is erroring. :(

I can reproduce once this PR is rebased on main

image

necolas avatar Feb 27 '24 18:02 necolas

@necolas Does this look good to merge? (I know you're on a break, so let me know if I should run this by someone else while you're away)

nmn avatar Mar 02 '24 05:03 nmn

There's still a few things to update before that. I'll comment over the weekend. Also left some questions above

necolas avatar Mar 02 '24 05:03 necolas

Does this patch still allow importing of types from the main package, e.g., import type { StrictHTMLElement } from 'react-strict-dom'?

Anything that is exported from the entry point can be imported directly. If there are types that are currently not exported from the entrypoint that are supposed to be public, we can add them.

nmn avatar Mar 02 '24 09:03 nmn

The paths to the build files also need to be updated in the performance workflow file.

^ this still needs to be done.

Also need to update the paths in the build size badges in the RSD README. They will update to the correct values once I publish the release after this patch

necolas avatar Mar 04 '24 18:03 necolas

I pushed fixes for the path updates.

However, there's one more path in the .flowconfig - https://github.com/facebook/react-strict-dom/blob/main/.flowconfig#L15. Updating that to point to the new flow entry point results in flow errors which should show up in CI for that commit. Removing the mapping (or mapping to a non-existent file, as was the case before in this PR) results in no flow errors...which is a bit concerning.

necolas avatar Mar 04 '24 19:03 necolas

Let me figure this out.

nmn avatar Mar 04 '24 22:03 nmn

Removing the mapping (or mapping to a non-existent file, as was the case before in this PR) results in no flow errors...which is a bit concerning.

This is a known issue with Flow. It causes "untyped" code rather than type errors. We might need to add some checks to cache type-coverage regressions for this. We also need to update the Flow version. I'll do that after this PR has merged.

nmn avatar Mar 06 '24 08:03 nmn

Thanks! Merged

necolas avatar Mar 07 '24 01:03 necolas