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

Updating data in canRoute should not removed any properties on target that are not available on source #160

Closed
mjstahl opened this issue Mar 17, 2018 · 7 comments

Comments

@mjstahl
Copy link

mjstahl commented Mar 17, 2018

In a donejs application is you have an app.js file that looks like:

import DefineMap from 'can-define/map/';
import route from 'can-route';
import 'can-route-pushstate';
import 'can-stache-bindings';

const ApplicationVM = DefineMap.extend({
  page: {
    type: 'string',
    get(currentPage) {
      return (this.session) ? (currentPage || 'foo') : 'login';
    },
    serialize: true
  },
  session: {
    type: 'any',
  },
});

route.register('{page}');
export default ApplicationVM;

In the console, run can.viewModel('html').session = {}. The expectation is that page would then be equal to foo.

It does not, because https://github.com/canjs/can-route/blob/master/can-route.js#L94 is calling canReflect.update where I think it should be calling canReflect.assign.

@justinbmeyer
Copy link
Contributor

Needs to call update. I think session should be serialize: false

@justinbmeyer
Copy link
Contributor

Needs to call update because if stateful properties in the url are removed when the url changes, they need to be removed from the view model.

@mjstahl
Copy link
Author

mjstahl commented Mar 17, 2018

Got it. That worked.

So what I think is a little wonky about this is:

  1. I don't get route properties displayed in the route unless I serialize: true
  2. Properties not on the route, but in the object need to be serialize: false otherwise they get deleted.

I think we should pick on and opt in. And in this case, I think that I am specifying a property in route.register (like route.register('{page}');) - page should be displayed in the route without serialize: true.

My preferred approach would be so:

  1. If I have properties registered via route.register
  2. If I have imported can-route-pushstate

then:

  1. I should NOT have to use serialize: true to get property values in the route
  2. I should NOT have to use serialize: false to keep properties from getting deleted when I update route.data

@mjstahl mjstahl closed this as completed Mar 17, 2018
@justinbmeyer justinbmeyer reopened this Mar 17, 2018
@justinbmeyer
Copy link
Contributor

You can use

“*”: {serialize: false}

To change the defaults.

I like the idea of making props identified by register as serialize:true no matter what.

I’m not sure what to do about point number 2. How should we know which props are part of the url and which aren’t if we support urls like /foo/bar?zed=ted ?

@justinbmeyer
Copy link
Contributor

We could show to use serialize:false trick to teach people how to opt in.

@justinbmeyer
Copy link
Contributor

Another way would be to mount to route data to a property within your app vm. This way everything on that will be serialize true. This might be a better idea.

@justinbmeyer
Copy link
Contributor

closing for #170

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

No branches or pull requests

2 participants