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

refactor($core): inject root mixins to root component and make LayoutDistributor functional #1231

Closed
wants to merge 4 commits into from

Conversation

meteorlxy
Copy link
Member

@meteorlxy meteorlxy commented Jan 26, 2019

Summary

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes (potentially)
  • No

If yes, please describe the impact and migration path for existing applications:

The clientRootMixin should be ssr compatible.

In fact, developers should always be aware of that in Vuepress, but our current updateMeta.js of core and mixin.js of docs are not so good at that.

So this is not an exact "breaking change".

Other information:

Based on #1228.

  • Inject root mixins to <Root> component, so they are "real" root mixins now.
  • Make LayoutDistributor functional. It would be lighter-weighted, and the structure of Vuepress App in vue-devtool would be much clearer.

Maybe we could have further discussion about this implementation @ulivz

import { getLayoutAsyncComponent } from '../util'

Vue.component('Layout', getLayoutAsyncComponent('Layout'))
Vue.component('NotFound', getLayoutAsyncComponent('NotFound'))
Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to move these two Vue.component() lines to app.js.

What do you think about that? @ulivz

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

Thanks but optimizing the codes that is already running very well doesn't make much sense, unless it does solve some of the problems, or it would bring potential issue risks.

Thanks again, and welcome to join in the official blog theme development if you want.

@ulivz ulivz closed this Jan 26, 2019
@meteorlxy
Copy link
Member Author

@ulivz I understand. I would like to express my opinions:


rootMixins is more reasonable to be injected to the root component, rather than to LayoutDistributor, because:

  • As its literally meaning
  • As how clientRootMixin explained in the docs:

    A path to the mixin file which allow you to control the life cycle of root component.

One possible usecase: user wants to add some properties to the root component via clientRootMixin, and accesses them via this.$root in some component. It's definitely different to inject them to LayoutDistributor.


About making LayoutDistributor functional:

  • LayoutDistributor does nothing but conditionally render component. It's a typical functional component.
  • Functional component is much cheaper to render
  • LayoutDistributor should be "transparent" to users, and functional component won't show up in the Vue devtools component tree, so that users would only see what they are using in Vue devtools
  • It's a preparation for future features like SPA application layout wrapper needed #1226 . If we could allow users to use custom LayoutDistributor, we should make our default LayoutDistributor as simple as possible. So we should move those Vue.component() codes out of it.

Seems like we usually have different opinions. Hope you can accept my opinion, but never mind if you declined. I just want to help make this project better 😉

@vuejs vuejs locked and limited conversation to collaborators Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants