Skip to content

b-button: Remove warning by importing b-link #201

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

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

yacinehmito
Copy link
Contributor

@yacinehmito yacinehmito commented Apr 4, 2017

Same drill as last time. (#174)

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks :) Just left some comments if you can fix them too.

@@ -18,7 +20,7 @@
];
},
componentType() {
return (this.href || this.to) ? 'b-link' : 'button';
return (this.href || this.to) ? bLink : 'button';
Copy link
Member

Choose a reason for hiding this comment

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

i think you have to keep this as is (b-link) as kebab case is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a string. It's a variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't take time to explain the reasoning as it's the same as #174

Copy link
Member

@mosinve mosinve Apr 5, 2017

Choose a reason for hiding this comment

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

@gpyh,
comparing to #174, this piece specially omitted or forgotten?

    components:{
        bLink
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually doesn't need the component to be registered as it uses directly its description.
It's still a good idea to add it though. I'll do that.

It used to embed the b-link component without importing it
@yacinehmito
Copy link
Contributor Author

yacinehmito commented Apr 6, 2017

Is something still blocking this PR?

I realize I didn't motivate the change properly. Allow me to fix this:

I use bootstrap-vue by importing each component manually instead of registering them in the global scope. It makes the final bundle smaller and the code more explicit.
The problem I encounter is the following:

Say I have a component A in my app that depends on a bootstrap-vue component called B, which itself depends on another bootstrap-vue component named C. The dependency graph looks roughly like this:

A (my own) -> B (bootstrap-vue) -> C (bootstrap-vue)

The code for A would be roughly this:

import { B } from 'boostrap-vue/lib/components'

export default {
  name: 'A',
  components: {
    B
  }
}

My app has no way of knowing that B depends on C. It can only work with the tag name. At this point that tag name is simply a string unrelated to any actual component.

Registering this transitive dependency in my components would look like this:

import { B, C } from 'boostrap-vue/lib/components'

export default {
  name: 'A',
  components: {
    B, C
  }
}

With this I can use C in A but the C in B still doesn't render. It's because the components are registered locally ; Vue doesn't walk up the components hierarchy. A component is either global or strictly local.

To fix this, bootstrap-vue needs to be explicit in the dependency it uses. It means importing and registering the appropriate components.

For #174 this was what I did. Here, the component is dynamically determined.
In that situation, you can either assign the component with its tag name, or directly with its description object.
That's why importing the component is needed, but not registering it.

I only made those fixes as the rendering errors appeared in my code, but in my opinion a better course of action would be to enforce this pattern across every component. I can carry out that task of reviewing all the components and making this fix if you want me too.

@pi0
Copy link
Member

pi0 commented Apr 6, 2017

but in my opinion a better course of action would be to enforce this pattern across every component.

@gpyh Thanks for your explanations and we totally agree about this to be done in entire components, The only question I had was that why we should point to reference instead of string b-link form? (We are already registring tag in components section :)

@pi0
Copy link
Member

pi0 commented Apr 6, 2017

Long time since it is open. Merging this as a starting point. Will discuss before migration other components.

@pi0 pi0 merged commit 752dd3a into bootstrap-vue:master Apr 6, 2017
pi0 pushed a commit that referenced this pull request Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants