-
Notifications
You must be signed in to change notification settings - Fork 905
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
[frameworks] better nuxt3 support #5490
base: master
Are you sure you want to change the base?
Conversation
- fixed some bugs for production build - added local dev support IMPORTANT: local dev only works if this pull request in superstatic will be merged -> firebase/superstatic#389
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Thanks for this @DevJoghurt, we'll take a look. |
PTAL @cfofiu |
src/frameworks/nuxt/index.ts
Outdated
* currently genrate is only supported by using nuxi generate or nuxi build --prerender | ||
* moreover, it is currently still experimental | ||
*/ | ||
const prerender = false; |
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.
We can turn this into a CLI experiment
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.
Prerender is one of several ways to build a nuxt app. This way the whole app is generated locally and no backend is needed. Other build options such as spa or ssr can be configured via config for individual routes, for which prerender is then set to false.
To use prerender as a whole app, you would need to use a cli argument, such as "firebase deploy --nuxt-preprender" or there is a way to specify prerender as config via firebase.json.
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.
Ah gotcha, I'll have to think about that more then...
I've been wondering if we should just shell out to npm run build
in all these integrations and change the default build step warning to something like we noticed your build step is not just "${default}". Here be dragons—do what you want—we expect the compiled artifacts to be in ${distDir} at the end of the day
. That way if they want prerender they just change the build step in their package.json.
Doing all the building in Node has theoretical benefits... but it seems the cost of maintaining these build steps and adding tons of framework specific options outweighs those. As an external contributor to our framework integrations do you agree or have any other feedback about our choice of abstraction?
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.
Yes, that makes perfect sense, as it would really be more maintenance free.
But of course it would also be possible to throw out the config for prerender, because the user can really overwrite it in the nuxt.config. The _generate
config only affects a complete static build of the page anyway. Building the app with different strategies is always possible on route level too.
Furthermore, with a build script it is no longer so easy to see if the nuxt app really needs a backend or not, since you don't have access to the configuration of nuxt.
I would of course find building the app via node nicer and also more practical but could of course understand the decision.
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.
Maybe I can implement both things. If the user defines a build script, then this is used. If there is no build script, the implemented node builder is used.
Here I would take out prerender and read via the generated build config of loadNuxt if a backend is needed or not.
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.
Let me and the team think on this strategy a bit before we reshuffle, in the meantime we'll review the rest of this PR.
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.
@DevJoghurt Is it possible to detect the prerender
config from a user's config/project setup?
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 think that it is not that simple. There is the configuration ssr: true
but that still allows another rendering configuration at route level. Only the separate nuxt cli command 'generate' allows the complete crawl-based pre-rendering as a static app. This command achieves the operation by simply overriding _generate via the loadNuxt function.
What would be the best course of action here, simply omit the static build of the app? This would mean that a backend is always needed for nuxt even if the user only wants to build a static app. Via the configuration generate -> routes he can still prerender single routes but the configuration of firebase would always be wantsBackend: true
A configuration via the firebase.json could still be an alternative.
@DevJoghurt Could you provide more details about what production build bugs this fixes? That will help a lot with review/testing. |
@austincrim All bugfixes for prod build: firebase-tools/src/frameworks/nuxt/index.ts Lines 64 to 79 in f70dad2
I have customized the build config for nuxt so that env variables are also added when building the app. Another todo here would be to remove the prerender config again, so that it can be configured by the user via the nuxt.config, as already discussed in the previous thread. firebase-tools/src/frameworks/nuxt/index.ts Lines 81 to 91 in f70dad2
These are minor changes that cover possible edge cases, such as deleting the output folder and writing the types. In principle, this is simply an adaptation of how nuxi (cli of nuxt 3) builds the app. firebase-tools/src/frameworks/nuxt/index.ts Line 110 in f70dad2
The output folder from the firebase build has caused me problems more often, so I think it's safer to delete it again before building the app. firebase-tools/src/frameworks/nuxt/index.ts Line 118 in f70dad2
The server build tool nitro from nuxt is tracing all packages and adds them as bundledDependencies to the package.json. Since the firebase cli performs an 'npm install', the packages added by the user and traced by nitro are uninstalled again. Therefore, the bundledDependencies must be added as dependencies. Added local dev with firebase emulator: firebase-tools/src/frameworks/nuxt/index.ts Lines 131 to 159 in f70dad2
For the local development via firebase emulator i also followed the implementation of nuxi (the cli of nuxt) and adapted it as far as possible to the firebase cli. In principle, most of the functions in utils.ts and prepare.ts are also necessary for this. As a todo here is still the automatic reload of the nuxt runtime on certain file changes. I would still add all the todos listed here if you let me know that the principle procedure in this push request is ok. |
@DevJoghurt thanks for the detail! We are moving forward with a different strategy for Nuxt dev mode (#5551) that spawns |
@austincrim |
@DevJoghurt thanks for your input on that PR! If you're still interested in moving forward with this PR, could you remove the dev mode related changes? If not, let us know and we can try to adapt your work into a separate PR, with full attribution. |
Thank you. |
@DevJoghurt @TheIronDev @cfofiu any movement on this? 🙏 |
Do you still have problems with the current firebase cli version? firebase-tools/src/frameworks/nuxt/index.ts Line 118 in f70dad2
The only problem I currently have is that when deploying packages are not installed, because nuxt takes bundledDependencies as a reference and this causes problems with the current procedure of the firebase cli. But I solve this with a custom nitro preset. Maybe @jamesdaniels can fix this last bug or the workflow of the firebase cli changes to take bundledDependencies into account. |
@DevJoghurt Yea that was the root of my issue too and basically the only thing that stopped it working "out of the box". I've also made a custom nitro preset to get around this. Here it is for anyone else who faces this issue: https://gist.github.com/JamieCurnow/374e69a2c3dd3b4952f967aa00a02938 I'm going to write up a little guide on Medium and link out to a demo minimal repo. Thanks for all your work and the comments around this! Such a great feature - especially having deploy previews of both front and backend with the pinTag feature 😍 |
I've ended up making a nuxt module to help with this: Feedback much appreciated 😊 |
Description
This pull request added better support for nuxt3.
IMPORTANT: local dev only works if this pull request in superstatic will be merged -> fs provider should return null if path has null bytes superstatic#389
Scenarios Tested
firebase emulator:start
firebase deploy
Sample Commands
firebase emulator:start
firebase deploy