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

Add compiler.define option #71802

Merged
merged 8 commits into from
Nov 12, 2024
Merged

Add compiler.define option #71802

merged 8 commits into from
Nov 12, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Oct 24, 2024

Adds a compiler.define option to next.config.js to statically replace any variables during build-time for Turbopack. This is similar to Webpack's DefinePlugin.

Relates to: #71476

@ijjk ijjk added Documentation Related to Next.js' official documentation. tests type: next labels Oct 24, 2024
@ijjk
Copy link
Member

ijjk commented Oct 24, 2024

Allow CI Workflow Run

  • approve CI run for commit: dc71558

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@lforst
Copy link
Contributor Author

lforst commented Oct 24, 2024

I just had the dumb realization that the env option already exists. https://nextjs.org/docs/pages/api-reference/next-config-js/env

It doesn't do the exact same thing but very often it might just be enough 🤔 Feel free to make the call that this PR is unnecessary.

@lforst
Copy link
Contributor Author

lforst commented Nov 6, 2024

It might actually not be a bad idea because the experimental. fallbackNodePolyfills option exists which will more or less break process.env usage (at least right now).

Encountered through user report in: getsentry/sentry-javascript#14193

@timneutkens
Copy link
Member

timneutkens commented Nov 7, 2024

Was discussing this with @sokra and @feedthejim, can you move the option to compiler.define instead and then also apply it when using webpack, that way it's the same between webpack/turbopack. I'm convinced we need this option next to env as it's being used in custom webpack config currently as well, e.g. to replace MY_VARIABLE stuff.

@lforst
Copy link
Contributor Author

lforst commented Nov 7, 2024

@timneutkens Makes sense! I moved the option to compiler and also use it for webpack builds 👍

@lforst lforst changed the title Add turbo.define option Add compiler.define option Nov 7, 2024
Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Awesome PR Luca!

@timneutkens timneutkens added the CI approved Approve running CI for fork label Nov 7, 2024
@ijjk
Copy link
Member

ijjk commented Nov 7, 2024

Tests Passed

Copy link
Contributor

@delbaoliveira delbaoliveira left a comment

Choose a reason for hiding this comment

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

Thank you for the docs! 🙏🏼

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Seems the define.test.ts added is failing during test runs 👀

@lforst
Copy link
Contributor Author

lforst commented Nov 8, 2024

Seems the define.test.ts added is failing during test runs 👀

Just saw. I have to look into it but either I am missing something completely obvious or there is something fundamentally off with the mechanics beneath what I am touching in this PR.

@timneutkens
Copy link
Member

Fixed the issue with the tests and opened a new bug report for Turbopack here: #72576. The issue was related to declare const.

@timneutkens timneutkens merged commit 1b1cb3e into vercel:canary Nov 12, 2024
85 of 86 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork Documentation Related to Next.js' official documentation. locked tests type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants