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

Route/Map triggering too many changes #2206

Closed
dylanrtt opened this Issue Jan 23, 2016 · 4 comments

Comments

Projects
None yet
4 participants
@dylanrtt
Contributor

dylanrtt commented Jan 23, 2016

I'm not sure if this can be reproduced without the can.route. If I just use a map, this bug does not occur.

http://jsbin.com/pajewebezu/edit?html,js,console,output

var appVM = new can.Map({});

can.route.map(appVM);
can.route.ready();

appVM.bind('action', function() {
  console.log('action change');
});

// triggers 'action' as expected
appVM.attr('action', 10);

setTimeout(function() {
  // unexpectedly triggers 'action' again
  appVM.attr('page', 'bar');
}, 100);

@dylanrtt dylanrtt changed the title from Route/Map Issue to Route/Map triggering too many changes Jan 23, 2016

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 25, 2016

Contributor

The reason for this is that the value is type coerced into strings: http://justinbmeyer.jsbin.com/megixa/edit?html,js,console,output

The only fix I can think of is that we overwrite .__set or maybe .__type on the route's map to automatically convert all values to strings.

Contributor

justinbmeyer commented Jan 25, 2016

The reason for this is that the value is type coerced into strings: http://justinbmeyer.jsbin.com/megixa/edit?html,js,console,output

The only fix I can think of is that we overwrite .__set or maybe .__type on the route's map to automatically convert all values to strings.

@justinbmeyer justinbmeyer added the bug label Jan 25, 2016

@justinbmeyer justinbmeyer added this to the 2.3.12 milestone Jan 25, 2016

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Jan 26, 2016

Contributor

Ah, so if I set with can.route.attr() instead of directly on appVM, the problem is avoided. Good to know.

The 2nd bug is that if I use can.route.attr(obj) with no 2nd argument, it does not stringify the object's properties because it incorrectly thinks it's a read. Reversing the order of the first two if statements should probably do the trick.

Contributor

dylanrtt commented Jan 26, 2016

Ah, so if I set with can.route.attr() instead of directly on appVM, the problem is avoided. Good to know.

The 2nd bug is that if I use can.route.attr(obj) with no 2nd argument, it does not stringify the object's properties because it incorrectly thinks it's a read. Reversing the order of the first two if statements should probably do the trick.

@daffl daffl modified the milestones: 2.3.13, 2.3.12 Jan 29, 2016

@daffl daffl modified the milestones: 2.3.15, 2.3.14 Feb 5, 2016

@nlundquist nlundquist self-assigned this Feb 9, 2016

@daffl daffl closed this in #2246 Feb 16, 2016

@dylanrtt

This comment has been minimized.

Show comment
Hide comment
@dylanrtt

dylanrtt Feb 18, 2016

Contributor

I think the overriden .__type method (and can.route.attr()) should ignore properties with serialize:false because there is no reason to stringify them, right? I ran into an issue with this fix where a boolean that the appVM handles internally was being stringified to "false" which changed the result of truthy checks.

Contributor

dylanrtt commented Feb 18, 2016

I think the overriden .__type method (and can.route.attr()) should ignore properties with serialize:false because there is no reason to stringify them, right? I ran into an issue with this fix where a boolean that the appVM handles internally was being stringified to "false" which changed the result of truthy checks.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 7, 2016

Contributor

@dylanrtt that's right. I'm thinking about how this should work for 3.0. My instinct is to simply have people default most properties to type: "string" like:

var AppVM = DefineMap.extend({
  "*": "string",
  somethingNotInTheRoute: {
    type: "boolean",
   serialize: false
  }
})
Contributor

justinbmeyer commented May 7, 2016

@dylanrtt that's right. I'm thinking about how this should work for 3.0. My instinct is to simply have people default most properties to type: "string" like:

var AppVM = DefineMap.extend({
  "*": "string",
  somethingNotInTheRoute: {
    type: "boolean",
   serialize: false
  }
})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment