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

[RFC] fix #4055, generate style on custom component, Don't Merge #4076

Merged
merged 4 commits into from Nov 4, 2016

Conversation

HerringtonDarkholme
Copy link
Member

The fix is discussed in #4055. I think attrs module should also have this fix.

@yyx990803
Copy link
Member

Can we add an accompanying test case in test/ssr/ssr-string.spec.js?

@HerringtonDarkholme
Copy link
Member Author

HerringtonDarkholme commented Oct 31, 2016

I'm adding tests. But this fix seems to have some problems. Still under investigation.

@HerringtonDarkholme HerringtonDarkholme changed the title fix #4055, generate style on custom component [WIP] fix #4055, generate style on custom component Oct 31, 2016
@HerringtonDarkholme HerringtonDarkholme changed the title [WIP] fix #4055, generate style on custom component [WIP] fix #4055, generate style on custom component, Don't Merge Oct 31, 2016
@HerringtonDarkholme
Copy link
Member Author

There is still a problem left. Code like this

{
  template: '<section><comp :style="style"></comp></section>',
  data: {
    style: 'color:red'
  },
  components: {
    comp: {
      template: '<div></div>'
    }
  }
}

Will produce code like this
<section server-rendered="true"><div></div></section>

Style is lost. This is because the inner template element div's vnode has no data. So the module logic isn't executed at all. I have no clear idea how to fix this...

}
}, result => {
expect(result).toContain(
'<div server-rendered="true" style="color:red"></div>'
Copy link
Member Author

Choose a reason for hiding this comment

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

<comp> has a synthetic server-rendered attrs so it has the chance to execute module logic...


// construct synthetic data for module processing
// because modules like style also produce code by parent VNode data
if (!node.data && hasAncestorData(node)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Add a synthetic data property if VNode's ancestor has data. I don't this is good enough...

@HerringtonDarkholme
Copy link
Member Author

Merge conflict resolved. My concern is the hasAncestorData check and a new node.data assignment.
@yyx990803 I think you are the best one to decide solution. Otherwise this pr is ready for code review.

@HerringtonDarkholme HerringtonDarkholme changed the title [WIP] fix #4055, generate style on custom component, Don't Merge [RFC] fix #4055, generate style on custom component, Don't Merge Nov 2, 2016
@yyx990803
Copy link
Member

Yeah, I think setting empty data object is fine.

It seems the style-generating code is only checking the direct parent node, where it's possible to have multi-nested components (A renders B, B renders C...), so we need to do a parent walk loop (while (parentNode = parentNode.parent) ...).

})
})

it('nested custom component style', done => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@yyx990803 By "multi-nested components", do you mean this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, the generateStyleText call is recursive, nice.

@yyx990803 yyx990803 merged commit 240df14 into vuejs:dev Nov 4, 2016
@yyx990803
Copy link
Member

I think this looks good. If the attrs module have a similar problem, we can do that in a separate PR.

@yyx990803
Copy link
Member

FYI attrs/domProps modules are fixed for similar situations in e5f23d9

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.

None yet

2 participants