Skip to content
This repository was archived by the owner on Apr 6, 2023. It is now read-only.
/frameworkPublic archive

fix(nuxt): ensure component dirs in node_modules have lower scanning priority - #6382

Merged
pi0 merged 4 commits into
nuxt:mainfrom
CodeDredd:main
Aug 9, 2022
Merged

fix(nuxt): ensure component dirs in node_modules have lower scanning priority#6382
pi0 merged 4 commits into
nuxt:mainfrom
CodeDredd:main

Conversation

@CodeDredd

@CodeDreddCodeDredd commented Aug 5, 2022

Copy link
Copy Markdown
Contributor

🔗 Linked issue

Related #6070

❓ 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

Somehow the behaviour for overwriting components which are defined by packages changed. For example if you have a Logo component which is stored under components/global will lose against the package like docus there also a component components/content/Logo is.

It gets even stranger. A component in components/global will be prefixed with Global which is not right. So at the end you get GlobalLogo registered

So for now it's not possible to overwrite a global component locally because of how the directories are listed. In my case the Logo from the node_modules package was always first and therefor no other defined component could overwrite it.

📝 Checklist

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

Reproduction

Priority issue: https://codesandbox.io/s/eager-noyce-r9tjgs

@netlify

netlifyBot commented Aug 5, 2022

Copy link
Copy Markdown

Deploy Preview for nuxt3-docs canceled.

NameLink
🔨 Latest commitd5f31ac
🔍 Latest deploy loghttps://app.netlify.com/sites/nuxt3-docs/deploys/62f2237b72f3a10009094b1a

@CodeDredd

CodeDredd commented Aug 5, 2022

Copy link
Copy Markdown
ContributorAuthor

@pi0 Now my chance is here for my first contribution 😄

@danielroe

Copy link
Copy Markdown
Member

Would you attach a reproduction showing that ~/components/global/SomeComponent.vue is prefixed with GlobalSomeComponent? 🙏

The linked PR (#6070) is still only available on the edge channel, and will not work in RC6.

@CodeDredd

Copy link
Copy Markdown
ContributorAuthor

@danielroe Sure....already working on it as mentioned. Yeah i know its edge. It's only releated to edge version

@CodeDredd

CodeDredd commented Aug 5, 2022

Copy link
Copy Markdown
ContributorAuthor

@danielroe Ok the thing with GlobalLogo i am not able to reproduce after running yarn upgrade.... i am trying to find out how i got it. Meanwhile the thing with the priority for components is still there.
Somehow i couldn't get a stackblitz example running. It throws me an arrow in @nuxthq

But here is a working reproduction what i mean:
https://github.com/CodeDredd/pinia-orm/tree/docs-search/docs
I have added a Logo in global and content and still the default Logo from docus is taken.

@CodeDredd

This comment was marked as off-topic.

@danielroe

Copy link
Copy Markdown
Member

Yes, it might be that you are on a version of edge before the PR, which is why a repro would be helpful.

I like the point about priority. But it might be more than just sorting them. See for example this logic in addComponent:

nuxt.hook('components:extend',(components: Component[])=>{
constexistingComponent=components.find(c=>(c.pascalName===component.pascalName||c.kebabName===component.kebabName)&&c.mode===component.mode)
if(existingComponent){
constname=existingComponent.pascalName||existingComponent.kebabName
console.warn(`Overriding ${name} component.`)
Object.assign(existingComponent,component)
}else{
components.push(component)
}
})
.

@CodeDredd

CodeDredd commented Aug 5, 2022

Copy link
Copy Markdown
ContributorAuthor

The global prefix issue:
Ok get your point....i can defintly tell you that you feature wasalready in the edge version....i am working on a reproduction.

The priority issue:
Ok i get what you mean....but somehow it doesnt come to that point if you run my repdroduction. Its never overwriting or coming to that hook because of this i think:

// Ignore component if component is already defined (with same mode)
if(!components.some(c=>c.pascalName===component.pascalName&&['all',component.mode].includes(c.mode))){
components.push(component)
}

@CodeDredd

CodeDredd commented Aug 5, 2022

Copy link
Copy Markdown
ContributorAuthor

Here is the priority issue thing. Please run yarn dev. somehow sandbox always uses ``yarn start`:
https://codesandbox.io/s/eager-noyce-r9tjgs

@CodeDredd

Copy link
Copy Markdown
ContributorAuthor

I give up on the prefix thing....seems to be working now. Renaming the ticket so its accurate

@pi0

pi0 commented Aug 5, 2022

Copy link
Copy Markdown
Member

Thanks for providing reproduction @CodeDredd 🙏🏼

Locally trying and debugging, this is normalized output of componentDirs:

- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/content [global] - /Users/pooya/tmp/r9tjgs/components/content [global]
- /Users/pooya/tmp/r9tjgs/components/global [global]
- /Users/pooya/tmp/r9tjgs/components
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/app [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/docs [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/prose [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/content [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/github [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt-themes/docus/components/icons [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxtjs/algolia/dist/runtime/components [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxt/content/dist/runtime/components [global]
- /Users/pooya/tmp/r9tjgs/node_modules/@nuxthq/studio/dist/runtime/components

Obviously, docus dir should be after yours to allow overriding. Let me debug what exactly is happening.

@CodeDreddCodeDredd changed the title fix(nuxt): components/global is not working correctlyfix(nuxt): local global components should have prority over package componentsAug 5, 2022
@pi0pi0 added the bug Something isn't working label Aug 5, 2022
@pi0

pi0 commented Aug 5, 2022

Copy link
Copy Markdown
Member

Debugging more, it seems a regression from the content module (See #1417) which we should solve there. (@CodeDredd please ping in linked comment if you like to work on fixing it. I think you can simply reverse layers() so that last unshift goes to first)

I think it would be a general good fix to ensure dirs with node_modules in them, always have lower priority during sorting process. What do you think @danielroe?

@pi0pi0 changed the title fix(nuxt): local global components should have prority over package componentsfix(nuxt): ensure component dirs in `node_modules have lower scanning priorityAug 5, 2022
@pi0pi0 changed the title fix(nuxt): ensure component dirs in `node_modules have lower scanning priorityfix(nuxt): ensure component dirs in node_modules have lower scanning priorityAug 5, 2022
@danielroe

Copy link
Copy Markdown
Member

Nice find. I agree, user provided components should always override modules/extends.

@pi0

pi0 commented Aug 5, 2022

Copy link
Copy Markdown
Member

js sorting is fun 😆

image

@CodeDredd

Copy link
Copy Markdown
ContributorAuthor

I dont't want make a double sort to ensure that global ist first content second and node_modules last . Damn js sort ^^

make sure that node_modules is always last and global dirs always first
@CodeDredd

Copy link
Copy Markdown
ContributorAuthor

@pi0 now you test it again....it should be now save that node_modules is always last and users global always first

pi0
pi0 approved these changes Aug 9, 2022
@pi0
pi0 merged commit 0b22079 into nuxt:mainAug 9, 2022
@pi0pi0 mentioned this pull request Aug 9, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3.xbugSomething isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@CodeDredd@danielroe@pi0