-
Notifications
You must be signed in to change notification settings - Fork 511
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
feat(azure): support custom configuration #1344
Conversation
β Live Preview ready!
|
Codecov Report
@@ Coverage Diff @@
## main #1344 +/- ##
==========================================
+ Coverage 76.39% 77.80% +1.40%
==========================================
Files 76 76
Lines 7790 7816 +26
Branches 784 797 +13
==========================================
+ Hits 5951 6081 +130
+ Misses 1837 1732 -105
- Partials 2 3 +1
|
@Hebilicious |
Hi @NicholasDawson , thank you for your hard work on this PR, and sorry for the time it took to review it. |
src/presets/azure.ts
Outdated
// Attempt to load custom config | ||
let customConfig = {}; | ||
try { | ||
customConfig = JSON.parse( |
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.
This can fail silently if JSON.parse fails (invalid json).
Maybe we should actually throw an error if JSON.parse fails ?
One way to do it would be to check if the file exist first.
const customConfig = exists ? JSON.parse() : {}
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 would get rid of the try-catch block and let the operation fail immediately instead of continuing with an invalid json
β¦asDawson/nitro into azure-preset-enhancements
@Hebilicious I made fixes based on your feedback, let me know what you think |
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 would get rid of the try catch block entirely.
Otherwise this looks good to me.
@Hebilicious All set. |
@Hebilicious do you know if this PR is ready to be merged, or does @pi0 need to review as well? Just wondering where it stands. |
Pi is the code owner and has to review and merge all non documentation PRs. |
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.
Thanks so much for the PR β€οΈ (and sorry it took long to review!)
I have made a couple of reactors to support via nitro.azure.config
instead to avoid introducing second file and be consistent with other presets i hope you are happy with it.
π Linked issue
#1087
β Type of change
π Description
I, along with many others deploying a Nitro server to Azure Static Web Apps, need to customize the
staticwebapp.config.json
file beyond what Nitro generates. Currently, Nitro overwrites the entire file on every compilation. It would be better if Nitro extended an existing configuration if present not overwrite the whole file.I added thorough documentation to explain how the system will now load and utilize the configuration. Please reference that to learn a little more about my implementation.
Implementation Notes
I put a lot of thought into this implementation as there wasn't necessarily a right or wrong way to handle this. I didn't originally want to have a separate file
custom.staticwebapp.config.json
, but the reason I had to add it was that if Nitro and the user are writing to the same file there is no way to keep track of what needs to take priority or be overwritten. Especially for routes, if Nitro generated the config for pre-rendered routes, but then the user deletes that pre-rendered route, it would be expected that the config removes that next time it compiles. If I tried to merge changes into thestaticwebapp.config.json
then Nitro would essentially lose the ability to remove lines from the configuration, only add them.Therefore it was necessary to introduce the convention of having the user add their own configuration to a file I called
custom.staticwebapp.config.json
to provide a clear distinction between what the user wants to add and what the final generated result (staticwebapp.config.json
) is.π Checklist