Skip to content
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

Merged
merged 21 commits into from
Jul 30, 2024
Merged

Migrate to Nuxt 3 #4257

merged 21 commits into from
Jul 30, 2024

Conversation

zackkrida
Copy link
Member

@zackkrida zackkrida commented May 2, 2024

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 in this PR
  • the large changes in patterns and structures
  • smallish changes that were required to migrate to Nuxt 3
  • enhancements that were not necessary for Nuxt 3 migration, but were useful and easy to make 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 uses Consola.
  • /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 and middleware/search.ts - the page component and the middleware responsible for fetching and displaying search results
  • pages/collection.vue and middleware/collection.ts - the page component and the middleware responsible for for fetching and displaying collection results
  • pages/image/[id]/index.vue, pages/audio/[id]/index.vue and middleware/single-result.ts - the page components and the middleware responsible for fetching and displaying the single result pages
  • Dockerfile

Infra-related questions:

  • Does this PR set up all the necessary env variables for the app to run (when we run 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 services were replaced with a single api-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 from useNuxtApp.

app settings and runtimeConfig

Added 2024-07-02
Previously, we often used the env variables, and manually added the env variables to the nuxt.config's env property. Here, we moved env properties to the public runtimeConfig.
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, calling useRuntimeConfig or useNuxtApp would often throw an error because it would not have access to the Nuxt context. The call to useNuxtApp would be nested in several layers of function calls (useFetch -> store's fetchMedia -> api-service's fetch function). However, with Nuxt 3.12, all of these errors disappeared, so I removed the try/catch guards from these calls.

env variable changes

Added 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 with NUXT_ prefix for private variables and NUXT_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:

# --------------------------------- #
# REQUIRED PRODUCTION ENV VARIABLES
# --------------------------------- #
# NUXT_PUBLIC_SENTRY_DSN=https://[email protected]/5799642
# NUXT_PUBLIC_SENTRY_ENVIRONMENT=production
# SITE_URL=https://openverse.org
# PLAUSIBLE_SITE_URL=https://openverse.org
# NUXT_PUBLIC_API_URL=https://api.openverse.org/
# NUXT_API_CLIENT_ID=
# NUXT_API_CLIENT_SECRET=

Fetching data in pages

The useFetch from @nuxtjs/composition-api was replaced with Nuxt 3 function useAsyncData in the search, collection and single-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 is error.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 the VHeaderMobile component to use a simpler component with VModalContent.

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 be string, number or boolean there. So, all the null values in analytics events were replaced with the string "null" in this PR.

import.meta

process.* was replaced with import.meta Link to Nuxt docs and JS docs

i18n 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 the layout and the middleware in a component that uses defineComponent imported from vue.
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 was template, followed by script.
In the follow-up issues, we should switch the order of script and template for components using script setup. We should also open an issue to convert the components to script setup syntax, which can be labeled as help 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 use UA anywhere.

splitting attrs to classes and other attrs

In Vue 2, the class and style 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 used inheritAttrs: 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 path id 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 from vueuse

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 keys

In v-for elements I replaced indexes with a better key option.

Disabled or not working items

  • Storybook (waiting for module update)
  • Prometheus
  • ua-parse
  • Storybook tests

Follow-up TODOs

  • Add structured logging to both the Nitro and Nuxt parts of the app (Nuxt structured logging #3435)
  • Move the script tag to the top of the components that use script setup syntax
  • Convert all components to script setup syntax (this should be a meta issue, and can be help wanted/good first issue)

@github-actions github-actions bot added 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend 🧱 stack: mgmt Related to repo management and automations labels May 6, 2024
@obulat obulat added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository labels May 6, 2024
@obulat obulat force-pushed the update/nuxt3-Apr26 branch 9 times, most recently from 14df293 to e63aaeb Compare May 9, 2024 09:30
@obulat obulat force-pushed the update/nuxt3-Apr26 branch 11 times, most recently from 6696e09 to 0b132cd Compare May 13, 2024 17:10
Copy link

github-actions bot commented May 13, 2024

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 🔄:

@obulat obulat force-pushed the update/nuxt3-Apr26 branch 2 times, most recently from eb1dc38 to f1935e3 Compare May 15, 2024 18:32
Comment on lines 19 to 25
const meta: Meta = [
{
key: "robots",
name: "robots",
content: "all",
},
]
Copy link
Member Author

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.

Copy link
Contributor

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",
Copy link
Member Author

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

@openverse-bot
Copy link
Collaborator

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@obulat
@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

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

  1. Specifically, Saturday and Sunday.

  2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range.

obulat and others added 19 commits July 30, 2024 07:16
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]>
* 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]>
Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@zackkrida zackkrida merged commit faf599b into main Jul 30, 2024
50 checks passed
@zackkrida zackkrida deleted the update/nuxt3-Apr26 branch July 30, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository ✨ goal: improvement Improvement to an existing user-facing feature 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend 🧱 stack: mgmt Related to repo management and automations
Projects
Archived in project
4 participants