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

[compiler] Disable emit of .tsbuildinfo #31459

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

nikeee
Copy link
Contributor

@nikeee nikeee commented Nov 8, 2024

Summary

@rollup/plugin-typescript emits a warning while building, hinting that outputToFilesystem defaults to true.

Although "noEmit" is set to true for the tsconfig, rollup writes a dist/.tsbuildinfo. That file is then also shipped inside the npm module and doesn't offer any benefit for library consumers. Setting this option to false results in the file not being written and thus omitted from the npm module.

How did you test this change?

dist/.tsbuildinfo is not emitted any more.

Copy link

vercel bot commented Nov 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 14, 2024 6:58pm

@poteto
Copy link
Member

poteto commented Nov 13, 2024

Thanks! Could you check if this slows down builds of the playground locally? We use the dist version of the compiler there for local development.

Alternatively we could prune this file from packages built in https://github.com/facebook/react/blob/main/compiler/scripts/release/publish.js so this is only removed for npm builds.

@nikeee
Copy link
Contributor Author

nikeee commented Nov 14, 2024

Good point!

yarn build in the playground calls build of babel-plugin-react-compiler, which does a rimraf on the dist/. But yarn dev calls rollup with --watch which doesn't rimraf dist. It boils down to what rollup --config --bundleConfigAsCjs --watch does.

During testing, I did not recognize any change in hot-reloading-speed in watch mode:

with dist/.tsbuildinfo:

[compiler] created dist/index.js in 10.9s
[compiler] bundles src/index.ts → dist/index.js...
[compiler] (!) Circular dependencies
[compiler] ...
[compiler] (!) [plugin typescript] @rollup/plugin-typescript: outputToFilesystem option is defaulting to true.
[compiler] created dist/index.js in 8.1s
[compiler] bundles src/index.ts → dist/index.js...
[compiler] (!) Circular dependencies
[compiler] ...
[compiler] (!) [plugin typescript] @rollup/plugin-typescript: outputToFilesystem option is defaulting to true.
[compiler] created dist/index.js in 8.6s

without dist/.tsbuildinfo:

[compiler] created dist/index.js in 11.3s
[compiler] bundles src/index.ts → dist/index.js...
[compiler] (!) Circular dependencies
[compiler] ...and 66 more
[compiler] created dist/index.js in 9.2s
[compiler] bundles src/index.ts → dist/index.js...
[compiler] (!) Circular dependencies
[compiler] ...and 66 more
[compiler] created dist/index.js in 8.9s
[compiler] bundles src/index.ts → dist/index.js...
[compiler] (!) Circular dependencies
[compiler] ...and 66 more
[compiler] created dist/index.js in 7s

(both builds fluctuate ~2s with the first build being slower)

Rollup still seems to have a tsconfig.tsbuildinfo inside .rollup.cache regardless of the setting. Maybe it uses that one.

Alternatively we could prune this file from packages built in main/compiler/scripts/release/publish.js so this is only removed for npm builds.

Possible, too. In that case we should set outputToFilesystem to true to get rid of the warning about true being the default.

What's your opinion?

@poteto
Copy link
Member

poteto commented Nov 14, 2024

Thanks for investigating! I think removing it just for npm builds is the way to go. While it might not affect build times, it may still be used for incremental rebuilds in dev as code is being edited. Would you mind modifying our compiler release script to include a Set of disallowed filenames from being included?

`@rollup/plugin-typescript` emits a warning while building,
hinting that `outputToFilesystem` defaults to true. To keep behavior and remove the warning, we just set it to `true`.

Although "noEmit" is set to `true` for the tsconfig, rollup writes a .tsbuildinfo.
The .tsbuildinfo is then also shipped inside the npm module and doesn't offer any benefit for library consumers.
@nikeee
Copy link
Contributor Author

nikeee commented Nov 14, 2024

While looking at the publish script, I realized that it may be cumbersome to add exclusion logic to the script. Instead, I changed files in package.json to exclude all .tsbuildinfo files.

Verified using ./scripts/release/publish.js --packages babel-plugin-react-compiler --debug:

npm notice Tarball Contents
npm notice 460B README.md
npm notice 5.1MB dist/index.js
npm notice 2.4kB package.json
npm notice Tarball Details

I changed outputToFilesystem to true (current default) to get rid of the warning, too.

@poteto
Copy link
Member

poteto commented Nov 14, 2024

Thank you!

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.

3 participants