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

docs: add treeshakeCompensation option to Next.js installation guide #177

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

mathieumetral
Copy link
Contributor

Why ?

Considering the significance of variables in StyleX, I suggest adding the treeshakeCompensation property to the installation steps for Next.js. Without this property, variables are removed during compilation as they are considered dead code. This can lead to confusion and difficulty for developers in setting up a project effortlessly.

@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 Dec 13, 2023
@nmn
Copy link
Contributor

nmn commented Dec 13, 2023

This change is not needed. treeshakeCompensation is always on in the nextjs plugin. treeshakeCompensation is something you need for some bundlers and is harmless in most others. There are very small edge-cases where you might need to turn it off. It only needs to be exposed and documented on the Babel level.

@nmn nmn closed this Dec 13, 2023
@mathieumetral
Copy link
Contributor Author

Thank you for the clarification! However, I feel there might be a subtle difference whether we explicitly set treeshakeCompensation or not, even though it's specified in the Next.js plugin. For instance, if I take the example app and remove treeshakeCompensation from the babelrc.js, I notice that variables like colors.blue3 are not defined.

With treeshakeCompensation
Screenshot 2023-12-13 at 11 27 18 PM

Without treeshakeCompensation
Screenshot 2023-12-13 at 11 26 48 PM

@nmn
Copy link
Contributor

nmn commented Dec 13, 2023

Yes. We want to remove the need for the .babelrc file entirely and this issue is already being tracked.

@nmn
Copy link
Contributor

nmn commented Dec 13, 2023

Thank you for the PR though. If you want to submit a PR specifically documenting the .babelrc requirement, i will accept that.

@mathieumetral
Copy link
Contributor Author

I'm having a little trouble understanding what is expected.

If I understand correctly, today, we need to have both a Babel config file and the Next.js plugin working together. However, going forward, the requirement for the Babel config file is anticipated to become unnecessary.

So, in the meantime, until the need for the Babel config file is phased out, should it be aligned with the configuration defined in the Next.js plugin (here)? And the change proposed in this PR aligns with that understanding.

Perhaps you would like me to add a comment explaining this temporary requirement?

@nmn nmn reopened this Dec 14, 2023
@nmn
Copy link
Contributor

nmn commented Dec 14, 2023

@mathieumetral This is my mistake.

@nmn nmn merged commit 5ee758d into facebook:main Dec 14, 2023
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.

3 participants