-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
fix(nuxt): use build plugin to detect usage of <NuxtPage>
and <NuxtLayout>
#26289
base: main
Are you sure you want to change the base?
fix(nuxt): use build plugin to detect usage of <NuxtPage>
and <NuxtLayout>
#26289
Conversation
Run & review this pull request in StackBlitz Codeflow. |
35fb406
to
732676a
Compare
74653a7
to
3aac698
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.
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.
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.
Should we remove the runtime nuxtApp._isNuxtLayoutUsed
since we analyse imports ?
edit: woops didn't see @danielroe 's comment
3aac698
to
2af6521
Compare
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.
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. π |
29a9f6c
to
92974b4
Compare
I have updated the PR with the discussed changes. π Things worth mentioning :
|
EDIT: nevermind, after running |
cf253a4
to
318717e
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.
Looking good!
<NuxtPage>
and <NuxtLayout>
@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 |
c0de392
to
18c37fb
Compare
I have updated the PR to take the feedback into account and to put the detection in a runtime plugin with an |
3b4b75a
to
04b42e7
Compare
6ed043a
to
d9d3393
Compare
d9d3393
to
4132d38
Compare
if (id[0] === '.') { | ||
id = join(importer, '..', id) | ||
} |
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 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?
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 } |
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.
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.)
In need of this, would love for it to get finished up. |
π Linked issue
Fixes #25912
β Type of change
π Description
The initial problem
Currently, the
check-if-layout-used
andcheck-if-page-unused
plugins check if their respective component has been mounted when the Nuxt App is ready. (withonNuxtReady()
). 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. useNuxtLayout
/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 theNuxtLayout
/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 theNuxtLayout
/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 theNuxtApp
: I could not add new properties to theNuxtApp
from the components module (which is where components are resolved for the whole app), and I could not access theNuxt
instance from thecheck-if-layout-used
andcheck-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 namedisNuxtLayoutUsed
detected-nuxt-page-usage.mjs
, which exports a const namedisNuxtPageUsed
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