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.

Sign up for GitHub

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

Jump to bottom

Added error when no config is exported from next.config.js #9921

Closed
wants to merge 7 commits into from
Closed

Added error when no config is exported from next.config.js #9921

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
184ba57
Added error when no config is exported from next.config.js
Jan 3, 2020
27185b8
Merge branch 'canary' into patch/fix-9919
jamesmosier Jan 3, 2020
7788fdf
Merge remote-tracking branch 'upstream/canary' into patch/fix-9919
Jan 3, 2020
cdeb9ef
Merge remote-tracking branch 'upstream/canary' into patch/fix-9919
Jan 7, 2020
975bd05
Merge remote-tracking branch 'upstream/canary' into patch/fix-9919
Jan 10, 2020
5461cd8
Merge remote-tracking branch 'upstream/canary' into patch/fix-9919
Jan 22, 2020
dd11ed7
added test for empty next.config file
Jan 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/next/next-server/server/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ export default function loadConfig(
phase,
userConfigModule.default || userConfigModule
)
if (Object.keys(userConfig).length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be checking for the number of keys in the object. Shouldn't we be checking for undefined (or a loose == null)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing, when I did

// some-config.js

{ someProp: true }
const config = require('./some-config.js') 
// config = {}

config was {}, not a falsy output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that I think of it, technically this is a valid next.config.js file...

module.exports = {}

so maybe there is another check I could do to see if the file has any exports defined 🤔

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 haven't been able to find a way to determine if a Node file actually contains a module.exports definition (short of reading the file via fs.readFile and parsing the string file contents).

There is a way to get the Node Module object of the file (via module.children.find), but the contents are still {} 😢

Any thoughts? I know that module.exports = {} is technically a valid next.config.js file, so checking via Object.keys isn't going to work under that assumption.

throw new Error(
'`next.config.js` file found, but nothing was exported from it'
)
}

if (userConfig.target && !targets.includes(userConfig.target)) {
throw new Error(
`Specified target is invalid. Provided: "${
Expand Down