fix(nuxt): use build plugin to detect usage of `<NuxtPage>` and `<NuxtLayout>` #26289

IonianPlayboy · 2024-03-15T22:41:44Z

🔗 Linked issue

❓ Type of change

📖 Documentation (updates to the documentation, readme or JSdoc annotations)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 Enhancement (improving an existing functionality like performance)
✨ New feature (a non-breaking change that adds functionality)
🧹 Chore (updates to the build process or auxiliary tools and libraries)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

The initial problem

Currently, the check-if-layout-used and check-if-page-unused plugins check if their respective component has been mounted when the Nuxt App is ready. (with onNuxtReady()). If this is not the case and the project has layouts/pages, they log a message in the console to warn the user to make them aware they should probably make some changes. (i.e. use NuxtLayout/NuxtPage in their app, or change their config)

This approach works well in most cases, but it raises a false positive if the NuxtLayout/NuxtPage components are not mounted right away, which can happen if there is a loading state that needs to be resolved before switching to the NuxtLayout/NuxtPage display. In that case, checking if the component is mounted is not enough to guarentee that it is not used in the app.

The idea

The best way to be sure that NuxtLayout/NuxtPage are indeed used in the user app, conceptually, is to check if they are imported in the user code. Since Nuxt is also responsible for configuring Vite/Webpack to load the app dependencies/modules, it makes sense to make use of it by checking if at some point Vite/Webpack has to resolve the NuxtLayout/NuxtPage components to determine if they are used or not.

However, I found it quite tricky to relay this information from the loader plugin all the way to the NuxtApp : I could not add new properties to the NuxtApp from the components module (which is where components are resolved for the whole app), and I could not access the Nuxt instance from the check-if-layout-used and check-if-page-unused plugins to hook into the Vite/Webpack config.

The solution

I ended up creating two virtual files with addTemplate to carry the data between the Nuxt and the NuxtApp instances:

detected-nuxt-layout-usage.mjs, which exports a const named isNuxtLayoutUsed
detected-nuxt-page-usage.mjs, which exports a const named isNuxtPageUsed

They are only created in dev mode (since the warnings are only intended for development) and are updated when the Vite/Webpach loader detect that the corresponding component is being imported. I thought about putting both variables inside a single virtual file, but decided against it because I was worried about concurrency issues.

Final thoughts

I am not 100% sure this is the best way to fix this issue, but with my current knowledge of Nuxt's codebase this is the best I could do on my own. I am interested in learning more about it though, so if there is anything to improve or maybe another solution that would be better than my current fix, feedback would be appreciated. 😊

📝 Checklist

I have linked an issue or discussion.
I have added tests (if possible).
I have updated the documentation accordingly.

stackblitz · 2024-03-15T22:41:47Z

Run & review this pull request in StackBlitz Codeflow.

packages/nuxt/src/components/loader.ts

danielroe

This is a nice idea. We're getting enough false positives on the warning that I wonder if it would be worth instead refactoring our implementation to use a compile-time check only rather than purely runtime.

The main issue here is that the NuxtLayout/NuxtPage might be detected across a project but not necessarily in the particular route being rendered. (Nevertheless, this is might still be fine because we're trying to check for the 'easy' case where a user is unaware they need to use NuxtPage/NuxtLayout rather than the more advanced cases.)

We'd also probably to update the implementation to detect any usage of NuxtLayout/NuxtPage that is imported (so not necessarily injected by components loader). It could be a separate vite plugin rather than adding into the existing component loader.

huang-julien

Should we remove the runtime nuxtApp._isNuxtLayoutUsed since we analyse imports ?

edit: woops didn't see @danielroe 's comment

IonianPlayboy · 2024-03-16T20:47:16Z

We're getting enough false positives on the warning that I wonder if it would be worth instead refactoring our implementation to use a compile-time check only rather than purely runtime.
...
We'd also probably to update the implementation to detect any usage of NuxtLayout/NuxtPage that is imported (so not necessarily injected by components loader). It could be a separate vite plugin rather than adding into the existing component loader.

This makes perfect sense to me. I tried to stay as close to the existing implementation as possible with my first attempt, but I agree that we should have all the data needed to emit an accurate warning without any runtime logic.

In the NuxtPage case, we could even add a check for a RouterView import to be even more precise in the warning message, since the main goal is to inform the user they should never have to use it in their Nuxt app.

I am going to try to update this PR to follow this new direction, and I'll report back if I encounter some issues or if I am stuck. 😊

IonianPlayboy · 2024-03-18T10:44:40Z

I have updated the PR with the discussed changes. 😊

Things worth mentioning :

I could not rely on app.layouts to detect if they were layouts in the user project, since we are deliberately ignoring index.vue layout here. I was not able either to find a way to import the layouts from #build/layouts inside core/nuxt.ts, so I ended up adding a new property to the app in order to check if they were any layout in the project or not. If there is a better way to do this, let me know and I'll change it.
The hasBeenInitialized check is necessary because when the app:templates run for the first time, the Vite/Webpack plugin has not been instancied yet - which means that we did not have the opportunity to detect anything yet. It works fine afterwards.
This Vite/Webpack plugin shares a lot of logic with the ImportProtectionPlugin. It might make sense to extract it into an utilsfunction, but I was not sure if this was relevant or not, where this should live and how it should be called.
Detecting RouterView was not as straightforward as I hoped, so I dropped the idea in the end.

packages/nuxt/src/core/nuxt.ts

IonianPlayboy · 2024-03-18T11:32:43Z

Ah, I just realized that this does not work without the setTimeout I was using in my playground to emulate loading, my bad. The app:templates hook is never triggered again in that case, which means that I never have the opportunity to check the component usage from core/nuxt.ts. I guess I'll have to use a plugin to do a runtime check then, and I'll remove the app._hasLayouts property since I will be able to use #build/layouts here.

EDIT: nevermind, after running pnpm cleanup and reinstalling everything it works now. I guess I did not update my local env properly after the latest changes from v3.11. Everything should be good to go now.

danielroe

Looking good!

packages/nuxt/src/core/app.ts

packages/nuxt/src/core/nuxt.ts

packages/nuxt/src/core/plugins/detect-component-usage.ts

IonianPlayboy · 2024-03-18T15:42:37Z

@danielroe I have found out that my current approach is flaky after trying a couple of scenarios. I have made some changes to use a runtime plugin that checks component usage on app:mountedto make it more robust. Can I push my changes here ?

IonianPlayboy · 2024-03-18T16:28:24Z

I have updated the PR to take the feedback into account and to put the detection in a runtime plugin with an app:mounted hook - this should be more robust than the previous approach. Let me know if there is anything to change. 🙂

danielroe · 2024-03-21T12:27:26Z

Don't worry - this is with us now to review. 🙏

…nted asynchronously

…when mounted asynchronously" This reverts commit 92974b4.

…untime

…sage

…ure" This reverts the following commits: fe0c830 eb80c31

…detection

danielroe · 2024-03-27T09:26:11Z

packages/nuxt/src/core/plugins/detect-component-usage.ts

+      if (id[0] === '.') {
+        id = join(importer, '..', id)
+      }


This logic doesn't quite look right... What about directories beginning with . - and if it's a relative path surely we could just do join(importer, id) rather than inserting an extra level up?

danielroe · 2024-03-27T09:28:50Z

packages/nuxt/src/core/plugins/detect-component-usage.ts

+      const isExcludedImporter = importersToExclude.some(p => typeof p === 'string' ? importer === p : p.test(importer))
+      const isIncludedImporter = importersToInclude.some(p => typeof p === 'string' ? importer === p : p.test(importer))
+      if (isExcludedImporter && !isIncludedImporter) { return }


Why do we need the include/exclude pattern? (If we do need it for some reason, we would probably need to add quite a few more directories, including all layer root directories, and the paths to all auto-imported components.)

rocketiscool · 2024-04-10T21:52:46Z

In need of this, would love for it to get finished up.

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from 35fb406 to 732676a Compare March 15, 2024 22:42

github-actions bot added 3.x bug labels Mar 15, 2024

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch 8 times, most recently from 74653a7 to 3aac698 Compare March 16, 2024 17:38

danielroe reviewed Mar 16, 2024

View reviewed changes

packages/nuxt/src/components/loader.ts Outdated Show resolved Hide resolved

danielroe reviewed Mar 16, 2024

View reviewed changes

huang-julien reviewed Mar 16, 2024

View reviewed changes

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from 3aac698 to 2af6521 Compare March 16, 2024 20:39

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch 3 times, most recently from 29a9f6c to 92974b4 Compare March 18, 2024 01:04

huang-julien reviewed Mar 18, 2024

View reviewed changes

packages/nuxt/src/core/nuxt.ts Outdated Show resolved Hide resolved

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from cf253a4 to 318717e Compare March 18, 2024 13:30

danielroe reviewed Mar 18, 2024

View reviewed changes

packages/nuxt/src/core/app.ts Outdated Show resolved Hide resolved

packages/nuxt/src/core/nuxt.ts Outdated Show resolved Hide resolved

packages/nuxt/src/core/plugins/detect-component-usage.ts Outdated Show resolved Hide resolved

danielroe changed the title ~~fix(nuxt): avoid warning about unused page/layout components when mounted asynchronously~~ fix(nuxt): use build plugin to detect usage of <NuxtPage> and <NuxtLayout> Mar 18, 2024

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from c0de392 to 18c37fb Compare March 18, 2024 16:26

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from 3b4b75a to 04b42e7 Compare March 18, 2024 22:11

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch 8 times, most recently from 6ed043a to d9d3393 Compare March 26, 2024 20:36

IonianPlayboy and others added 16 commits March 27, 2024 00:52

fix(nuxt): avoid warning about unused page/layout components when mou…

e89240c

…nted asynchronously

Revert "fix(nuxt): avoid warning about unused page/layout components …

dc46308

…when mounted asynchronously" This reverts commit 92974b4.

feat(nuxt): add _hasLayouts property on NuxtApp

e94e25e

refactor(nuxt): emit component usage warnings at compile time

b8ea5de

refactor(nuxt): remove plugins with runtime component usage checks

cefa4c0

[autofix.ci] apply automated fixes

488f2bb

refactor(nuxt): use addBuildPlugin to add DetectComponentUsagePlugin

6f54ebb

chore: remove old code

2330bc9

fix: use app.layouts as source of truth

10fbc4c

refactor(nuxt): use check-component-usage plugin to log warnings at r…

8163fe4

…untime

refactor(nuxt): use absolute path with distDir for detect-component-u…

06ff4df

…sage

refactor(nuxt): hardcode sep character

aa4eda9

test: fix runtime-compiler fixture

9e58137

test: fix basic fixture

0f986b0

Revert "test: fix runtime-compiler fixture" and "test: fix basic fixt…

4a9609d

…ure" This reverts the following commits: fe0c830 eb80c31

fix(nuxt): include the generated runtime app.vue for component usage …

4132d38

…detection

IonianPlayboy force-pushed the fix/unused-layout-page-warning branch from d9d3393 to 4132d38 Compare March 26, 2024 23:52

refactor: simplify template

8145b40

danielroe reviewed Mar 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(nuxt): use build plugin to detect usage of `<NuxtPage>` and `<NuxtLayout>` #26289

fix(nuxt): use build plugin to detect usage of `<NuxtPage>` and `<NuxtLayout>` #26289

IonianPlayboy commented Mar 15, 2024

stackblitz bot commented Mar 15, 2024

danielroe left a comment

huang-julien left a comment •

edited

IonianPlayboy commented Mar 16, 2024 •

edited

IonianPlayboy commented Mar 18, 2024 •

edited

IonianPlayboy commented Mar 18, 2024 •

edited

danielroe left a comment

IonianPlayboy commented Mar 18, 2024

IonianPlayboy commented Mar 18, 2024

danielroe commented Mar 21, 2024

danielroe Mar 27, 2024

danielroe Mar 27, 2024

rocketiscool commented Apr 10, 2024

fix(nuxt): use build plugin to detect usage of <NuxtPage> and <NuxtLayout> #26289

Are you sure you want to change the base?

fix(nuxt): use build plugin to detect usage of <NuxtPage> and <NuxtLayout> #26289

Conversation

IonianPlayboy commented Mar 15, 2024

🔗 Linked issue

❓ Type of change

📚 Description

The initial problem

The idea

The solution

Final thoughts

📝 Checklist

stackblitz bot commented Mar 15, 2024

danielroe left a comment

Choose a reason for hiding this comment

huang-julien left a comment • edited

Choose a reason for hiding this comment

IonianPlayboy commented Mar 16, 2024 • edited

IonianPlayboy commented Mar 18, 2024 • edited

IonianPlayboy commented Mar 18, 2024 • edited

danielroe left a comment

Choose a reason for hiding this comment

IonianPlayboy commented Mar 18, 2024

IonianPlayboy commented Mar 18, 2024

danielroe commented Mar 21, 2024

danielroe Mar 27, 2024

Choose a reason for hiding this comment

danielroe Mar 27, 2024

Choose a reason for hiding this comment

rocketiscool commented Apr 10, 2024

fix(nuxt): use build plugin to detect usage of `<NuxtPage>` and `<NuxtLayout>` #26289

fix(nuxt): use build plugin to detect usage of `<NuxtPage>` and `<NuxtLayout>` #26289

huang-julien left a comment •

edited

IonianPlayboy commented Mar 16, 2024 •

edited

IonianPlayboy commented Mar 18, 2024 •

edited

IonianPlayboy commented Mar 18, 2024 •

edited