-
Notifications
You must be signed in to change notification settings - Fork 108
Set App Hosting overrides for NextJS apps without a Next Config #323
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with some comments
opts.projectDirectory, | ||
originalConfig.configFileName, | ||
userNextConfigExists, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not too sure about passing this extra userNextConfigExists, if we can avoid it. and since the value of userNextConfigExists
determines which blocks of code get executed in each function, i think that means we can re-org the code a little so it's more predictable what happens in each function without having to read each line.
My initial question is do we even need to validate our own default / provided nextjs config? If not I think we could do something much more similar to the original code. Maybe:
const userNextConfigExists = await exists(join(root, originalConfig.configFileName));
if (userNextConfigExists) {
await overrideNextConfig(root, originalConfig.configFileName);
await validateNextConfigOverride(root, opts.projectDirectory, originalConfig.configFileName);
} else {
// call some new function to write the DEFAULT_NEXT_CONFIG_FILE
}
Alternatively (my slight preference), we just do:
await overrideNextConfig(root, originalConfig.configFileName);
await validateNextConfigOverride(root, opts.projectDirectory, configFileName);
but overrideNextConfig()
now handles two additional things:
(1) whether userNextConfigExists (and then writing DEFAULT_NEXT_CONFIG_FILE if not)
(2) ensuring that the preservedConfigFile still exists (instead of doing this in the validate function)
Then we don't have to pass around userNextConfigExists, validateNextConfig does not have to care if this is the original, if userconfigexists, etc.
also (2) relies on some details like knowing the original file prefix next.config.original...
. So I think it would be good to keep any logic that requires knowing that prefix contained to one place.
you've thought about this more and I may be missing some challenges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial question is do we even need to validate our own default / provided nextjs config? If not I think we could do something much more similar to the original code. Maybe:
maybe excessive, but more importantly I don't think we need to validate the existence of the next.config.original.[js|ts|mjs]
file as it is a redundant check. loadConfig
will error out if for whatever reason the next.config.orignal... file doesn't exist when user's config is being overriden.
if (!userNextConfigExists) { | ||
console.log(`No Next config file found, creating one with Firebase App Hosting overrides`); | ||
try { | ||
await writeFile(join(projectRoot, DEFAULT_NEXT_CONFIG_FILE), defaultNextConfigForFAH()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does nextConfigFileName
include the default filename? Then we can drop nextConfigFileName
in favor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve, nit in comment
Currently we're skipping adding FAH overrides for Next projects that don't have a Next Config file.