-
Notifications
You must be signed in to change notification settings - Fork 202
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
Migrate to Nuxt 3 #4257
Migrate to Nuxt 3 #4257
Conversation
14df293
to
e63aaeb
Compare
6696e09
to
0b132cd
Compare
Full-stack documentation: https://docs.openverse.org/_preview/4257 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
eb1dc38
to
f1935e3
Compare
5fca1fd
to
582cb2d
Compare
frontend/src/utils/og.ts
Outdated
const meta: Meta = [ | ||
{ | ||
key: "robots", | ||
name: "robots", | ||
content: "all", | ||
}, | ||
] |
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.
Is this necessary? I thought by default the seo robots plugin was adding the correct robots meta tags to pages and that we could avoid setting these values outside of the nuxt config. That was the paradigm I tried to embrace in #4659, at least.
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 noticed that I incorrectly reversed the seo
Playwright test changes from the single result fix PR. So, when fixing this, I re-added this value here.
Then I tested the set up again, and realized that the robots setup used a trailing slash for disallow
routes and therefore was creating incorrect meta tags and robots settings. Updated in the latest commit. I'm not sure what the difference between the value all
and the default value used by robots module is.
"vitest": "^2.0.4", | ||
"vitest-dom": "^0.1.1", | ||
"vue": "3.4.33", | ||
"vue-router": "^4.4.0", |
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.
Just noting: This release includes a partial fix for the memory leak issue! vuejs/router#2137
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @zackkrida, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]> Clean up env variables Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
* Fix robots.txt for Nuxt 3 * Update frontend/nuxt.config.ts
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
b8b84be
to
3715639
Compare
Signed-off-by: Olga Bulat <[email protected]>
9f0b3ef
to
b7f3d0b
Compare
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.
🎉
Fixes
Fixes #411
Fixes #3106
Description
Originally, this PR description contained information on how to deploy Nuxt 3 to staging. This was incorporated in the PR. To view them, look at the history of this comment.
Information described here:
The most important items to review
/server
directory contains the server routes (healthcheck and robots), as well as the server-side Sentry and server-side logger that usesConsola
./data/api-service
- the code used for requesting data from Openverse API.app.vue
- the single entry point to the frontend app.nuxt.config.ts
- static configuration file for the app, plugins and modules.pages/search.vue
andmiddleware/search.ts
- the page component and the middleware responsible for fetching and displaying search resultspages/collection.vue
andmiddleware/collection.ts
- the page component and the middleware responsible for for fetching and displaying collection resultspages/image/[id]/index.vue
,pages/audio/[id]/index.vue
andmiddleware/single-result.ts
- the page components and the middleware responsible for fetching and displaying the single result pagesDockerfile
Infra-related questions:
just start
in production)? Probably should be solved in an accompanying infra PR (if any changes needed)Changed code patterns and structures
API service
All the various
service
s were replaced with a singleapi-service
that has only the methods that we use. The extra methods that were not used by the frontend (e.g.,put
,update
,delete
) were removed.The API service uses
useRuntimeConfig
for the API URL, and uses the API token fromuseNuxtApp
.app settings and
runtimeConfig
Added 2024-07-02
Previously, we often used the
env
variables, and manually added theenv
variables to thenuxt.config
'senv
property. Here, we movedenv
properties to the publicruntimeConfig
.With Nuxt 3, we use
runtimeConfig
to keep track of app configuration such as the API URL. Before I updated Nuxt 3 to the latest version, callinguseRuntimeConfig
oruseNuxtApp
would often throw an error because it would not have access to the Nuxt context. The call touseNuxtApp
would be nested in several layers of function calls (useFetch
-> store'sfetchMedia
-> api-service'sfetch
function). However, with Nuxt 3.12, all of these errors disappeared, so I removed thetry/catch
guards from these calls.env
variable changesAdded 2024-07-15
Nuxt 3 sets up the
runtimeConfig
values using the env variables provided when running the app. The convention is to use a variable withNUXT_
prefix for private variables andNUXT_PUBLIC_
prefix for public variables. Currently,nuxt.config
sets some of the variable defaults for local development.It will be necessary to provide the following env variables to the production build:
Fetching data in pages
The
useFetch
from@nuxtjs/composition-api
was replaced with Nuxt 3 functionuseAsyncData
in thesearch
,collection
andsingle-result
pages use the for fetching the data.All of the server-side fetching for search, collection and single result pages is done in the middleware.
I tried moving the fetching to the
useAsyncData
to unify and simplify it. However, this causes client-server mismatch because the fetching is done in the sub-page, after the surrounding components such as header and footer have already been rendered. This means that the result count is always set to 0 in the header.When hydrating, the client expects it to be the real number because the store has the updated data, and the header is being rendered with that. We could wrap the changing items with
<ClientOnly>
to prevent the mismatch, but it's much easier to fetch the data in the middleware, before the app starts rendering, and have the correct data rendered on the server everywhere.Single entry point,
app.vue
Nuxt 3 introduces a new single entry point,
app.vue
. This is used to add the functionality that should work whenever a user gets a server-rendered page, wether it's a homepage, a search page, a single result page or a static page.Previously, we used the global middleware for the functions that do not require the app to be mounted and do not require a window access. For the functions that need the app to be mounted (such as updating the layout values with the cookies or syncing values with local storage), we added them to every layout file.
Layout files became much simpler because most functionality was moved to
app.vue
Note: the only page not using the
app.vue
iserror.vue
. We must duplicate the i18n properties and the layout update from cookies there.Trailing slashes
After @sarayourfriend's comment on trailing slashes, I reviewed our approach to trailing slashes in the frontend. Previously, our approach was slightly inconsistent. The only route that had a trailing slash was the "All media" search path, which had a trailing slash between the path and the query (
https://openverse.org/search/?q=cat
). All other paths did not have trailing slashes.In this Nuxt 3 branch, the trailing slash from the search route was removed, so now no routes have trailing slashes.
Modals
Vue 3 and Nuxt 3 natively support teleports, so the modals were updated to use the native support. We no longer need a modal target because Nuxt automatically creates a
#teleports
element.8655ced in Vue version 3.4.32 broke the way
VInputModal
was rendering the recent searches modal on mobile on SSR. This is why I rewrote theVHeaderMobile
component to use a simpler component withVModalContent
.Smaller changes required for Nuxt 3
Plausible types don't allow
null
The new Plausible plugin uses
plausible-tracker
JS library, and the event properties can only bestring
,number
orboolean
there. So, all thenull
values in analytics events were replaced with the string"null"
in this PR.import.meta
process.*
was replaced withimport.meta
Link to Nuxt docs and JS docsi18n keys
i18n
now checks for the correctness of the keys. So, if there are strings with non-ASCII keys (e.g., "text of the {translated keys containing non-ascii letters}"), the build fails. This is why I added a quick fix for this. It should be improved later (and the translations need to be updated in GlotPress)script setup for pages
All pages were converted to
script setup
because it's impossible to set thelayout
and themiddleware
in a component that usesdefineComponent
imported fromvue
.The new Vue 3 convention is to have
script
tag at the top of the component. In this PR, we keep to the previous convention when the top tag wastemplate
, followed byscript
.In the follow-up issues, we should switch the order of
script
andtemplate
for components usingscript setup
. We should also open an issue to convert the components toscript setup
syntax, which can be labeled ashelp wanted
.SVG sprite
SVG sprite module was vendored in because it did not work with our folder structure, where we have a
/src
top folder.ua-parse
ua-parse
was removed because the library it uses does not have ESM, and we don't useUA
anywhere.splitting attrs to classes and other attrs
In Vue 2, the
class
andstyle
were not passed to the child component as part of$attrs
, but instead were added to the outermost element of the child component. This broke the styles of the components that usedinheritAttrs: false
(VSearchBar
,VSelectField
,VItem
).Enhancements (not-necessary but useful)
Media item preprocessing
We already pre-process the API responses to add necessary properties (
frontendMediaType
) or to fix encoding/decoding problems.I added the extraction of the filetype from the main media URL. This way, we don't have to replace the filetype on the client-side after the image/audio have been loaded. This removes the flickering and also the occasional client-server mismatch.
UUID check
Added a
validateUUID
check to the single result page to directly show an error page if the pathid
parameter is not a valid UUID instead of attempting to fetch it.DevEx improvement
The server API requests are logged in the console to simplify debugging.
Replacing ad-hoc implementations with Vueuse
use-window-scroll
composable was replaced by the composable fromvueuse
Homepage images issue
Fixed the issue when the image on a single result page was not updated if you selected one from the homepage, went back and selected another one.
v-for
keysIn
v-for
elements I replaced indexes with a better key option.Disabled or not working items
Follow-up TODOs
script
tag to the top of the components that usescript setup
syntaxscript setup
syntax (this should be a meta issue, and can behelp wanted
/good first issue
)