automatic merging of next.config.js files #222
Conversation
scefali
requested review from
a team,
lforst and
Lms24
and removed request for
a team
There was a problem hiding this comment.
@roggenkemper I pulled a random example online and it doesn't look like we can handle it, is it expected for this config?
module.exports = {
images: {
domains: [],
},
}
|
Just jumping in to give some early feedback. From our experience, mucking with the AST is super super fragile. If I were to tackle this I would probably look into some more robust (but uglier) solutions first. We could even change the signature and functionality of |
|
@lforst What kinds of robust solutions are you thinking of? We are open to ideas. That being said, I think this approach could do the job. Just need to write about ~10 unit tests to cover different formats that we commonly see and I think this will work 50-90% of the time. Even if we can't make this work 100% of the time...anything better than 0% is an improvement over today. |
|
made some updates based off the conversation with Luca - the flow now is to not deal with AST's, but instead essentially copy over their original config, but add in the call with withSentryConfig which will be updated (getsentry/sentry-javascript#6721) to allow us to add in the Sentry options that we need to add into their config. |
roggenkemper
requested review from
scefali and
lforst
and removed request for
lforst
| 'Please merge those files.', | ||
| ); | ||
| nl(); | ||
| if (templateFile === 'next.config.js') { |
There was a problem hiding this comment.
the config file could also be .ts
There was a problem hiding this comment.
is this an edge case that needs to be handled? it seems rare and currently we would create a next.config.js file that they would be able to merge.
lib/Helper/MergeConfig.ts
Outdated
| const templateFile = fs.readFileSync(templatePath, 'utf8'); | ||
| const sourceFile = fs.readFileSync(sourcePath, 'utf8'); | ||
| const newText = templateFile.replace('// ORIGINAL CONFIG', sourceFile); | ||
| new Function(newText); // check if the file is valid javascript |
There was a problem hiding this comment.
I don't think this will throw if you pass in invalid JS. I would just remove this line.
There was a problem hiding this comment.
In my tests, it seems to work as I intended (if there is a syntax error then it will throw an error and the function will return false) . I think eval would achieve the same thing. @scefali mentioned wanting to have some kind of syntax check here
There was a problem hiding this comment.
Ah you're right. I messed up how I verified this. Sorry about that!
| ): Promise<void> { | ||
| // if no next.config.js exists, we'll create one | ||
| if (!fs.existsSync(destinationPath)) { | ||
| fs.copyFileSync(templatePath, destinationPath); |
There was a problem hiding this comment.
The written file is gonna be a bit ugly because of the // ORIGINAL CONFIG comment. Can we either replace it with something a little more instructive or remove it before writing it to disk?
We should probably replace it even with
module.exports = {
// Your Next.js configuration here
}
or smth like that.
There was a problem hiding this comment.
If no next.config.js exists, then it will use the existing next.config.js template instead of the new one with the // ORIGINAL CONFIG line.
There was a problem hiding this comment.
Ah you're right. I hope this isn't just me but to me the entire logic of this file is super hard to get behind. We should def clean this up some time (not your code but the one that existed before - it probably just grew over time).
This pr adds in automatic merging of the next.config.js files. If there is no existing next.config.js file, then we will add the existing default next.config.js file in. If we are able to merge Sentry's with the existing, then the file will be updated (with the original saved as next.config.original.js). If we are unable to merge, we will create a template file and ask them to merge (the status quo approach) .