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] Update next.config.js link to default config #22989

Merged
merged 3 commits into from
Mar 17, 2021

Conversation

jflayhart
Copy link
Contributor

@jflayhart jflayhart commented Mar 11, 2021

It's currently misleading and I chose to freeze to a particular version since pointing to canary every time is likely to break (as is happening prior to this PR).

The link will now go to: https://github.com/vercel/next.js/blob/v10.0.8/packages/next/next-server/server/config-shared.ts#L33

@jflayhart jflayhart changed the title Update next.config.js link to default config [Docs] Update next.config.js link to default config Mar 11, 2021
@@ -44,7 +44,7 @@ module.exports = (phase, { defaultConfig }) => {
}
```

The commented lines are the place where you can put the configs allowed by `next.config.js`, which are defined [here](https://github.com/vercel/next.js/blob/canary/packages/next/next-server/server/config.ts#L12-L63).
The commented lines are the place where you can put the configs allowed by `next.config.js`, which are defined [here](https://github.com/vercel/next.js/blob/v10.0.8/packages/next/next-server/server/config-shared.ts#L33).
Copy link
Member

@lfades lfades Mar 11, 2021

Choose a reason for hiding this comment

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

It's not likely we'll change the location of the configs very often, but the configs are more likely to change, so it's okay to point it to canary

Suggested change
The commented lines are the place where you can put the configs allowed by `next.config.js`, which are defined [here](https://github.com/vercel/next.js/blob/v10.0.8/packages/next/next-server/server/config-shared.ts#L33).
The commented lines are the place where you can put the configs allowed by `next.config.js`, which are defined [here](https://github.com/vercel/next.js/blob/canary/packages/next/next-server/server/config-shared.ts#L33).

Copy link
Member

Choose a reason for hiding this comment

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

It might be good to add a comment above this line in the actual code noting we need to update this doc if the location does end up changing in the future.

Copy link
Member

Choose a reason for hiding this comment

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

We should document all the options in the docs instead imo.

Copy link
Contributor Author

@jflayhart jflayhart Mar 17, 2021

Choose a reason for hiding this comment

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

@lfades I committed your suggestion

We should document all the options in the docs instead imo.

@timneutkens if we do that, then we have to update configs in both places: docs and code. Wouldn't it be easier to just maintain a link as a pointer to the code's config object? To @lfades point, the location of the config should rarely change, but properties on the config might (ie experimental stuff); therefore a link to the config is more "future proof".

Co-authored-by: Luis Alvarez D. <luis@vercel.com>
@jflayhart jflayhart requested a review from lfades March 17, 2021 19:11
Copy link
Member

@lfades lfades 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!

@kodiakhq kodiakhq bot merged commit 34e9409 into vercel:canary Mar 17, 2021
@jflayhart jflayhart deleted the patch-1 branch April 2, 2021 19:48
flybayer pushed a commit to blitz-js/next.js that referenced this pull request Apr 29, 2021
It's currently misleading and I chose to freeze to a particular version since pointing to canary every time is likely to break (as is happening prior to this PR).

The link will now go to: https://github.com/vercel/next.js/blob/v10.0.8/packages/next/next-server/server/config-shared.ts#L33
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants