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

feat(nuxt): warn when page uses a layout without <NuxtLayout> #24116

Merged
merged 16 commits into from Nov 23, 2023

Conversation

luc122c
Copy link
Contributor

@luc122c luc122c commented Nov 4, 2023

πŸ”— Linked issue

#19392

❓ 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

Resolves #19392 by adding a warning in development if layouts are present but the <NuxtLayout> component has not been used. All changes are dev-only and should be tree-shaken out.

We first add a new variable nuxtApp.payload._isNuxtLayoutUsed which is false by default. When the <NuxtLayout> component is declared, this is switched to true. A new plugin is added which runs onNuxtReady. If Nuxt Layout has not been used, then it counts the layouts registered and if this is greater than 0, displays a warning. #build/layouts provides the layouts in the src directory, as well as in any layers.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Nov 4, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@luc122c
Copy link
Contributor Author

luc122c commented Nov 4, 2023

Agh, I commited to my main branch 😞

@luc122c luc122c changed the title feat(dx): warn when a page uses a layout but no <NuxtLayout> is created feat(dx): warn when a page uses a layout but no <NuxtLayout> is created Nov 4, 2023
@luc122c luc122c changed the title feat(dx): warn when a page uses a layout but no <NuxtLayout> is created feat(nuxt): warn when a page uses a layout but no <NuxtLayout> is created Nov 7, 2023
@luc122c luc122c marked this pull request as ready for review November 7, 2023 23:29
@luc122c
Copy link
Contributor Author

luc122c commented Nov 7, 2023

Please could the CI be re-run, I think it's an unrelated timeout.

Copy link
Member

@danielroe danielroe left a 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 improvement πŸ‘Œ

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests βœ…

❗ No coverage uploaded for pull request base (main@db74ead). Click here to learn what that means.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24116   +/-   ##
=======================================
  Coverage        ?   58.76%           
=======================================
  Files           ?        5           
  Lines           ?      861           
  Branches        ?       46           
=======================================
  Hits            ?      506           
  Misses          ?      355           
  Partials        ?        0           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@danielroe danielroe changed the title feat(nuxt): warn when a page uses a layout but no <NuxtLayout> is created feat(nuxt): warn when page uses a layout without <NuxtLayout> Nov 23, 2023
@danielroe danielroe merged commit 4ce6bc2 into nuxt:main Nov 23, 2023
33 checks passed
@github-actions github-actions bot mentioned this pull request Nov 23, 2023
@nathanchase
Copy link
Contributor

Question: what about if you only ever declare your layouts for your page components in definePageMeta - is this no long recommended? It doesn't seem "in error" if your layouts are all declared this way, is it?

definePageMeta({
  layout: 'user-layout',
});

@danielroe
Copy link
Member

@nathanchase That should be totally fine and is recommended. What is the issue you are experiencing?

@nathanchase
Copy link
Contributor

@nathanchase That should be totally fine and is recommended. What is the issue you are experiencing?

With the nightly build, I was noticing the warning that I had layout files but didn't use <NuxtLayout>.

@danielroe
Copy link
Member

Were you visiting a page with a layout defined in metadata, but not using <NuxtLayout>?

@nathanchase
Copy link
Contributor

Correct.

@danielroe
Copy link
Member

Well, then that's exactly what this PR is meant to warn against. You have defined a layout for a page but aren't displaying that layout. Or am I missing something?

@nathanchase
Copy link
Contributor

No, I have the layout defined in the meta tag, but is that not enough to indicate that the page should be automatically wrapped by that layout? Are you meant to additionally add a <NuxtLayout layout="user-layout"></NuxtLayout> around your page component, as well as add it to definePageMeta?

@danielroe
Copy link
Member

danielroe commented Dec 22, 2023

No, just <NuxtLayout><NuxtPage /></NuxtLayout> is enough. NuxtLayout defaults to the layout specified by the current route.

And this isn't a change. It's not automatically added, which is why we created this PR. Layouts can be used on their own or separately so you need to add it to your app.vue when you start defining them in your route metadata or nothing will happen.

@nathanchase
Copy link
Contributor

What about nested layouts?

Example:
I have a default.vue layout in /layouts.

Then I have another layout (/layouts/user-layout.vue) that is wrapped like so:

<NuxtLayout name="default">
...
</NuxtLayout>

Then in a page component, I reference the user-layout with just definePageMeta:

definePageMeta({
  layout: 'user-layout',
});

This has worked without issue until now.

@danielroe
Copy link
Member

Perhaps you had better create a new issue with a reproduction. This PR did not change the actual behaviour at all, only added some console logging to avoid users expecting to see a layout but not seeing one because they forgot to add <NuxtLayout> to their app. But that doesn't seem to be your situation.

@nathanchase
Copy link
Contributor

Perhaps it is the absence of an app.vue altogether?

@danielroe
Copy link
Member

I still don't understand the issue you're having.

@nathanchase
Copy link
Contributor

nathanchase commented Dec 22, 2023

The problem is that I've been using layouts for months and it all works as expected, but this PR seems to think I was doing it incorrectly and sends a warning that I'm not using NuxtLayout, even though I am using <NuxtLayout name="default"> in several layout files. Will try to reproduce it. Just wanted to raise the flag here in case it was an issue with how this was determining the warning.

@danielroe
Copy link
Member

So you are using it.

That sounds like a bug in how we are detecting usage, then. Could you create a reproduction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn when a page uses a layout but no <NuxtLayout> is created
5 participants