Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

refactor(nuxt): explicitly import app in nuxt-root #8729

Merged
merged 1 commit into from Nov 9, 2022
Merged

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This is a micro optimisation. We likely don't need access to <App> outside of <NuxtRoot> and it feels like having it in the vue component registry isn't necessary. But I may have missed something here - wdyt @antfu?

πŸ“ Checklist

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

@danielroe danielroe requested a review from antfu November 5, 2022 13:51
@danielroe danielroe self-assigned this Nov 5, 2022
@codesandbox
Copy link

codesandbox bot commented Nov 5, 2022

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Nov 5, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit 81ffdf3
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/63666a4db791630008d4c1af

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Sounds reasonable to me

@pi0
Copy link
Member

pi0 commented Nov 7, 2022

Having App auto import could be useful for custom Root component implementations and also mocking purposes. I think we could rename it to NuxtApp BTW.

Any specific reason made you to make this refactor @danielroe ?

Copy link
Member Author

The reason for the refactor is that storing the component in the vue component registry is a slight additional overhead when it's only used once.

For custom root components, we could import directly from #build/app-component.mjs. And it might be easier to add support later for a future use case if we need to do it. And there are some challenges. Really if used, it should be <NuxtRoot> which provides the base <Suspense>. There's no guarantee that the app component will work on its own. At the moment, I think this is an edge case and I know you'd be the first to ask for a really solid use case before enabling. (If we do want to add this feature, I would prefer to use addComponent which would import at point-of-use.)

Regarding name, I think <NuxtApp> would be confusing as it's not the same thing as nuxtApp . <AppComponent> or merely <App> works - but again, it's not quite the same as a normal App.vue in a non-Nuxt project.

@pi0
Copy link
Member

pi0 commented Nov 9, 2022

I don't think there is any noticeable overhead of registering one component to the Vue interface (otherwise we have really issues with scaling any Nuxt project with dozens of components...)

Naming NuxtAppComponent makes sense but sure let's add it when we officially support a way to customize Root component. Removing public component until then makes sense πŸ‘πŸΌ

@pi0 pi0 merged commit 3839dba into main Nov 9, 2022
@pi0 pi0 deleted the refactor/app-import branch November 9, 2022 09:14
@pi0 pi0 mentioned this pull request Nov 15, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants