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

fix: add treeshakeCompensation to esbuild plugin #457

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

nedjulius
Copy link
Contributor

@nedjulius nedjulius commented Feb 18, 2024

What changed / motivation ?

This PR adds treeshakeCompensation: true to stylex babel plugin in esbuild plugin to avoid elision of modules. It also adds an esbuild plugin test case for TypeScript files.

Linked PR/Issues

Fixes #454

Pre-flight checklist

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 18, 2024
@nmn
Copy link
Contributor

nmn commented Feb 19, 2024

Thanks for the quick fix to documentation, but could you instead setting treeshakeCompensation: true in the ESBuild plugin instead? That should fix this problem without needing an extra option for devs to configure.

@nedjulius
Copy link
Contributor Author

@nmn will add. thanks for letting me know 👍

@nedjulius nedjulius changed the title docs: add tsconfig setup info to esbuild-plugin docs fix: add treeshakeCompensation to esbuild plugin Feb 19, 2024
@nedjulius
Copy link
Contributor Author

updated the PR

Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

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

Changes look good, but there is a failing test.

@@ -115,6 +115,7 @@ export default function stylexPlugin({
jsxSyntaxPlugin,
stylexBabelPlugin.withOptions({
...options,
treeshakeCompensation: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, you should make treeshakeCompensation an optional argument that defaults to true.

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 think moving it above ...options should suffice?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this should work.

@nmn nmn merged commit 58fe43e into facebook:main Feb 21, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@stylexjs/esbuild-plugin] The esbuld-plugin does not embed variables in the TypeScript build.
3 participants