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

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

Closed
wants to merge 10 commits into from

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Feb 24, 2024

#Fixes #25

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

  • Copy the gen-flow wrapper around flow-api-translator
  • Set up script to generate per-file Flow and TS type-defs
  • Update the package.json to point to the new output files
  • 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 nmn marked this pull request as draft February 24, 2024 05:37
Copy link

github-actions bot commented Feb 24, 2024

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%) 🔴

@necolas
Copy link
Contributor

necolas commented Feb 24, 2024

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

@nmn
Copy link
Contributor Author

nmn commented Feb 26, 2024

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.

@necolas
Copy link
Contributor

necolas commented Feb 26, 2024

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

@nmn nmn marked this pull request as ready for review February 26, 2024 04:31
@nmn
Copy link
Contributor Author

nmn commented Feb 26, 2024

@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
Copy link
Contributor Author

nmn commented Feb 26, 2024

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.

packages/scripts/gen-types.js Outdated Show resolved Hide resolved
packages/scripts/rewrite-imports.js Outdated Show resolved Hide resolved
packages/scripts/rewrite-imports.js Outdated Show resolved Hide resolved
@nmn
Copy link
Contributor Author

nmn commented Feb 27, 2024

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

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +129 to +132
file: path.join(
__dirname,
'../packages/react-strict-dom/dist/dom/index.js'
),
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the www build? That is still being written to dist/dom.www.js; how is internal use going to work if the types are being picked up from the new flow files but they have references to the @stylexjs/stylex package instead of stylex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be transforming the imports while copying the files over during sync.

@necolas
Copy link
Contributor

necolas commented Feb 27, 2024

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

@nmn nmn force-pushed the chore/build-script branch from fecb50c to 7a09ab6 Compare March 1, 2024 11:55
@nmn
Copy link
Contributor Author

nmn commented Mar 2, 2024

@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)

@necolas
Copy link
Contributor

necolas commented Mar 2, 2024

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

@nmn
Copy link
Contributor Author

nmn commented Mar 2, 2024

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.

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

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

.gitignore Outdated Show resolved Hide resolved
packages/react-strict-dom/src/dom/runtime.js Show resolved Hide resolved
packages/scripts/package.json Outdated Show resolved Hide resolved
packages/scripts/package.json Show resolved Hide resolved
packages/scripts/package.json Outdated Show resolved Hide resolved
packages/scripts/rewrite-imports.js Show resolved Hide resolved
packages/scripts/handle-flow-modules.js Outdated Show resolved Hide resolved
packages/scripts/gen-types.js Outdated Show resolved Hide resolved
packages/scripts/gen-types.js Show resolved Hide resolved
@necolas
Copy link
Contributor

necolas commented Mar 4, 2024

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
Copy link
Contributor

necolas commented Mar 4, 2024

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 necolas force-pushed the chore/build-script branch from fda70f0 to 01c6f32 Compare March 4, 2024 19:28
@nmn
Copy link
Contributor Author

nmn commented Mar 4, 2024

Let me figure this out.

@nmn
Copy link
Contributor Author

nmn commented Mar 6, 2024

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.

@necolas necolas closed this in 7e2ecc2 Mar 7, 2024
@necolas
Copy link
Contributor

necolas commented Mar 7, 2024

Thanks! Merged

@nmn nmn deleted the chore/build-script branch March 7, 2024 10:58
@necolas necolas mentioned this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript support
3 participants