fix: ensure nextjs picks up the correct config file when called by nextjs_standalone_build()#2778
Conversation
|
MatrixFrog
changed the title
|
@MatrixFrog after testing this more and asking robots to write tests or better solutions I think what you have here might be as good as we'll get, at least without a lot of extra complexity. Was there anything else you wanted to do here before merging? |
I mean, probably, since I left it in draft state, but I don't remember what 🙃 so I guess go ahead and merge this if you're happy with it. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4cf30d169
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Well my remaining concerns:
But I can't think of any (simple) way around this. We could have a simple check for files in So I think your change is a dx improvement even if not perfect and even without automated tests 🤷 |
|
Yeah it's definitely a bit weird. If I were the king of all frontend-dom I would make it so |
Next.js discovers config files in priority order: next.config.js > next.config.mjs > next.config.ts. When the user's config is named next.config.js, it was loaded directly by the CLI, bypassing the next.bazel.mjs wrapper and leaving .aspect_rules_js symlinks in the standalone output. Fix (from aspect-build#2778): rename the user's config to __original.<basename> so Next.js cannot discover it. Use config_basename extracted from the label to handle label forms like :next.config.js or //pkg:next.config.js. Add a fail() at analysis time if the config file also appears directly in srcs or data, which would reintroduce the bypass even with the rename. Add e2e tests covering four config variants (next.config.js CJS, next.config.js ESM via "type":"module", next.config.mjs, next.config.cjs) with sh_test regression tests that verify standalone/node_modules contains no .aspect_rules_js symlinks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Next.js discovers config files in priority order: next.config.js > next.config.mjs > next.config.ts. When the user's config is named next.config.js, it was loaded directly by the CLI, bypassing the next.bazel.mjs wrapper and leaving .aspect_rules_js symlinks in the standalone output. Fix (from aspect-build#2778): rename the user's config to __original.<basename> so Next.js cannot discover it. Use config_basename extracted from the label to handle label forms like :next.config.js or //pkg:next.config.js. Add a fail() at analysis time if the config file also appears directly in srcs or data, which would reintroduce the bypass even with the rename. Add e2e tests covering four config variants (next.config.js CJS, next.config.js ESM via "type":"module", next.config.mjs, next.config.cjs) with sh_test regression tests that verify standalone/node_modules contains no .aspect_rules_js symlinks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Next.js discovers config files in priority order: next.config.js > next.config.mjs > next.config.ts. When the user's config is named next.config.js, it was loaded directly by the CLI, bypassing the next.bazel.mjs wrapper and leaving .aspect_rules_js symlinks in the standalone output. Fix (from aspect-build#2778): rename the user's config to __original.<basename> so Next.js cannot discover it. Use config_basename extracted from the label to handle label forms like :next.config.js or //pkg:next.config.js. Add a fail() at analysis time if the config file also appears directly in srcs or data, which would reintroduce the bypass even with the rename. Add e2e tests covering four config variants (next.config.js CJS, next.config.js ESM via "type":"module", next.config.mjs, next.config.cjs) with sh_test regression tests that verify standalone/node_modules contains no .aspect_rules_js symlinks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Next.js discovers config files in priority order: next.config.js > next.config.mjs > next.config.ts. When the user's config is named next.config.js, it was loaded directly by the CLI, bypassing the next.bazel.mjs wrapper and leaving .aspect_rules_js symlinks in the standalone output. Fix (from aspect-build#2778): rename the user's config to __original.<basename> so Next.js cannot discover it. Use config_basename extracted from the label to handle label forms like :next.config.js or //pkg:next.config.js. Add a fail() at analysis time if the config file also appears directly in srcs or data, which would reintroduce the bypass even with the rename. Add e2e tests covering four config variants (next.config.js CJS, next.config.js ESM via "type":"module", next.config.mjs, next.config.cjs) with sh_test regression tests that verify standalone/node_modules contains no .aspect_rules_js symlinks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jbedard
force-pushed
the
tyler/next-fix
branch
3 times, most recently
from
d488e49 to
6436a7c
Compare
|
@MatrixFrog does the additional test and minor change I pushed look ok? It will be squashed with your commit when this merges... |
|
cc @tyler-breisacher-zipline so that I'll get an email about this on my other account |
jbedard
changed the title
3 participants
Without this change, there are two next config files available to the
nextbinary: the one the user passed in, and next.bazel.mjs, which has been renamed tonext.config.mjsby copy_file(). We want it to use the next.bazel.mjs one as the config file, but if the one from the user is named next.config.js, then it gets picked up by the next CLI instead.By renaming the user's config (prefixing it with
__original), we ensure thatnextwill pick up the one we want it to.Fix #2789
Changes are visible to end-users: yes
Test plan