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

Allow templatizer to be used without owner or host prop forwarding. Fixes #4458 #5035

Merged
merged 13 commits into from
Jan 29, 2018

Conversation

kevinpschaaf
Copy link
Member

@kevinpschaaf kevinpschaaf commented Jan 10, 2018

Makes templatizer safe to be used without an owner (only needed when forwarding props to/from host via options.forwardHostProp and automatically hiding/showing children if the owner is hidden). If no owner is used, default template processing and binding evaluation from PropertyEffects will be used (normally it delegates to any overrides in the owner). Also no-ops dispatchEvent so that path notifications when not forwarding to a host does not throw.

In this change, filtering the user props bag on options.instanceProps was removed since it seems not strictly required.

Reference Issue

Fixes #4458
Fixes #3422

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

The documentation issue listed in the first comment of #4458 has not been resolved yet. In this preview of the docs, the documentation of the function does not mention all options are required.

@kevinpschaaf
Copy link
Member Author

@TimvdLippe See #4458 (comment), basically this PR makes it work with only one argument, the other two are now actually optional where they were not before. Whoops, I didn't mark the 2nd owner argument optional; making that change now.

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Ah okay. Thanks for the clarification @kevinpschaaf !

}
}
for (let hprop in this.__hostProps) {
this._setPendingProperty(hprop, this.__dataHost['_host_' + hprop]);
for (let iprop in props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment saying that passing in a prop in props that has the same name as a bound host prop may shadow it. This is considered a user error (warn?). We should add this to the docs too.

@TimvdLippe
Copy link
Contributor

Could we check if this PR also fixes #5063? Maybe that can be fixed at the same time. If not, we have to separate the work.

@sorvell sorvell merged commit a73fdd7 into master Jan 29, 2018
@sorvell sorvell deleted the 4458-kschaaf-basic-templatizer branch January 29, 2018 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants