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

Move where %root is added to a component’s initial viewModel data #2282

Merged
merged 5 commits into from Mar 1, 2016

Conversation

Projects
None yet
3 participants
@chasenlehara
Member

chasenlehara commented Feb 22, 2016

When a component is set up, %root was added to the initial viewModel data so it would be available in every scope. However, can.view.Scope already has a special case for %root, so it’s not necessary for can.Component to do that.

Fixes #2250

chasenlehara added some commits Feb 21, 2016

Don’t include %root in a component’s initial viewModel data
When a component is set up, `%root` was added to the initial `viewModel` data so it would be available in every scope. However, [`can.view.Scope` already has a special case for %root](https://github.com/canjs/canjs/blob/cee94ae7744ea6376e1f4bd7f6088b325db06555/view/scope/scope.js#L91-L94), so it’s not necessary for `can.Component` to do that.

Fixes #2250

@chasenlehara chasenlehara added the bug label Feb 22, 2016

@chasenlehara chasenlehara added this to the 2.3.18 milestone Feb 22, 2016

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Feb 22, 2016

Contributor

It's necessary for people that do this.attr("%root") in a ViewModel though. Before can-wait we were telling people to do:

var promise = SomeModel.getList();
this.attr("%root").waitFor(promise);
return promise;

Does this break something for you?

Contributor

matthewp commented Feb 22, 2016

It's necessary for people that do this.attr("%root") in a ViewModel though. Before can-wait we were telling people to do:

var promise = SomeModel.getList();
this.attr("%root").waitFor(promise);
return promise;

Does this break something for you?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 22, 2016

Contributor

Scopes have it, but not component viewModels. This needs to be present for ssr.

Sent from my iPhone

On Feb 22, 2016, at 12:24 AM, Chasen Le Hara notifications@github.com wrote:

When a component is set up, %root was added to the initial viewModel data so it would be available in every scope. However, can.view.Scope already has a special case for %root, so it’s not necessary for can.Component to do that.

Fixes #2250

You can view, comment on, or merge this pull request online at:

#2282

Commit Summary

Add failing test for component/define/type/list/map issue
Don’t include %root in a component’s initial viewModel data
File Changes

M component/component.js (4)
M component/component_test.js (37)
Patch Links:

https://github.com/canjs/canjs/pull/2282.patch
https://github.com/canjs/canjs/pull/2282.diff

Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Feb 22, 2016

Scopes have it, but not component viewModels. This needs to be present for ssr.

Sent from my iPhone

On Feb 22, 2016, at 12:24 AM, Chasen Le Hara notifications@github.com wrote:

When a component is set up, %root was added to the initial viewModel data so it would be available in every scope. However, can.view.Scope already has a special case for %root, so it’s not necessary for can.Component to do that.

Fixes #2250

You can view, comment on, or merge this pull request online at:

#2282

Commit Summary

Add failing test for component/define/type/list/map issue
Don’t include %root in a component’s initial viewModel data
File Changes

M component/component.js (4)
M component/component_test.js (37)
Patch Links:

https://github.com/canjs/canjs/pull/2282.patch
https://github.com/canjs/canjs/pull/2282.diff

Reply to this email directly or view it on GitHub.

@chasenlehara

This comment has been minimized.

Show comment
Hide comment
@chasenlehara

chasenlehara Feb 22, 2016

Member

Thanks guys. Based on what I found, I think I could fix this by just moving that line of code. I’ll write a test for %root first, make sure this fix breaks it, then see if I can move that line of code to satisfy both tests.

@matthewp, yes, this commit is what first started breaking my test case for #2250. I determined that when %root was being set up, it was not correctly initializing my scope variable.

Member

chasenlehara commented Feb 22, 2016

Thanks guys. Based on what I found, I think I could fix this by just moving that line of code. I’ll write a test for %root first, make sure this fix breaks it, then see if I can move that line of code to satisfy both tests.

@matthewp, yes, this commit is what first started breaking my test case for #2250. I determined that when %root was being set up, it was not correctly initializing my scope variable.

chasenlehara added some commits Feb 23, 2016

Wait to add %root to a component’s viewModel data until all of the ot…
…her attributes have been bound

When a component is set up, `%root` is added to the initial `viewModel` data so it’s available in every scope. However, this causes issues when the `%root` bindings are set up before all of the other properties, so this moves the setup for `%root` until after all the other properties have been bound.

Fixes #2250
@@ -130,6 +128,9 @@ steal("can/util", "can/view/callbacks","can/view/elements.js","can/view/bindings
if(setupBindings) {
teardownFunctions.push(bindings.behaviors.viewModel(el, componentTagData, function(initialViewModelData){
// Make %root available on the viewModel.
initialViewModelData["%root"] = componentTagData.scope.attr("%root");

This comment has been minimized.

@justinbmeyer

justinbmeyer Mar 1, 2016

Contributor

I'm not sure how this is working because it's not adding it to initialViewModelData._data

@justinbmeyer

justinbmeyer Mar 1, 2016

Contributor

I'm not sure how this is working because it's not adding it to initialViewModelData._data

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 1, 2016

Contributor

@chasenlehara I'm curious why the initial data would cause the breaking behavior you identified. It doesn't make sense to me, which makes me think there's a better fix somewhere.

Also, you should be able to demonstrate this w/o a component. Just create the VM in the same way can.Component does, with initial data like new YourMap({"%root": new can.Map()})

Contributor

justinbmeyer commented Mar 1, 2016

@chasenlehara I'm curious why the initial data would cause the breaking behavior you identified. It doesn't make sense to me, which makes me think there's a better fix somewhere.

Also, you should be able to demonstrate this w/o a component. Just create the VM in the same way can.Component does, with initial data like new YourMap({"%root": new can.Map()})

justinbmeyer added a commit that referenced this pull request Mar 1, 2016

Merge pull request #2282 from canjs/2250-component-type
Don’t include %root in a component’s initial viewModel data

@justinbmeyer justinbmeyer merged commit 6ceaa66 into master Mar 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@justinbmeyer justinbmeyer deleted the 2250-component-type branch Mar 1, 2016

@chasenlehara

This comment has been minimized.

Show comment
Hide comment
@chasenlehara

chasenlehara Mar 1, 2016

Member

The cause of this issue is a little tough to explain, but here’s my best shot:

  • can.Map helpfully keeps track of objects that it’s converted, so if you pass it an object twice, it’ll return the same can.Map instance
  • When %root is a plain object, it gets converted to a can.Map

In #2250, %root has a collection that is converted into a can.List because %root is converted first. Then, when collection is defined on the component, can.Map (unhelpfully in this case) returns the can.List it had already created instead of converting the object to the special Collection type.

This PR “fixes” (it’s kind of a hack) the issue by setting up %root after the other properties are converted, so it references the converted values instead of creating its own.

@justinbmeyer and I talked about this and he might be able to add more clarification to this rough description.

Member

chasenlehara commented Mar 1, 2016

The cause of this issue is a little tough to explain, but here’s my best shot:

  • can.Map helpfully keeps track of objects that it’s converted, so if you pass it an object twice, it’ll return the same can.Map instance
  • When %root is a plain object, it gets converted to a can.Map

In #2250, %root has a collection that is converted into a can.List because %root is converted first. Then, when collection is defined on the component, can.Map (unhelpfully in this case) returns the can.List it had already created instead of converting the object to the special Collection type.

This PR “fixes” (it’s kind of a hack) the issue by setting up %root after the other properties are converted, so it references the converted values instead of creating its own.

@justinbmeyer and I talked about this and he might be able to add more clarification to this rough description.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 1, 2016

Contributor

@chasenlehara good explanation.

Contributor

justinbmeyer commented Mar 1, 2016

@chasenlehara good explanation.

@chasenlehara chasenlehara changed the title from Don’t include %root in a component’s initial viewModel data to Move where %root is added to a component’s initial viewModel data Mar 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment