These are chat archives for canjs/canjs

25th
Jul 2018
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 15:47
Hi I've found an issue using CanMap ..attr to set multiple properties at once
Here is a link to a page that shows the problem
Basically the problem is when we set a prop value with map.attr({"prop1": "new value"})
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:02
Is this supposed to still be supported on CanJS latest version?
Justin Meyer
@justinbmeyer
Jul 25 2018 17:23
looking at it
what is the problem?
expectation vs reality?
only one event is being fired?
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:25
exactly
with version can@3.8.0 the event was being triggered twice
Justin Meyer
@justinbmeyer
Jul 25 2018 17:26
hmmm, it seemed to switch to doing a merge ...
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:27
Is it somehow related with the canReflect also?
Justin Meyer
@justinbmeyer
Jul 25 2018 17:28
yeah, almost certainly
can.Reflect.assign(myMap1Instance,{
  "prop1": new MyMap({prop1: "xyz"})
})
that fixes this example
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:29
this is a workaround, right?
The previous way is supposed to still be supported, right?
Justin Meyer
@justinbmeyer
Jul 25 2018 17:30
yes, this is a workaround. Yes, the previous way should still be supported.

so apparently can-define and can-map work a bit different here and our tests didn't cover that difference.

Essentially, we made canReflect do what can-define does, and then made can-map use canReflect

but clearly there were some differences that were not hit by tests
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:32
should this be resolved on issue canjs/can-map#111 ? Or it should be a new one?
Justin Meyer
@justinbmeyer
Jul 25 2018 17:32
I'm creating a different issue
To be honest, I'm wary of changing the behavior back a bit ...
I almost feel like the best solution for you is to create a can-map-legacy that has the old behavior
and you could use that
but the problem with this is that can-map-define directly imports can-map
So the other "fix" might be to make some sort of toggle you can switch when defining maps
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:35
I hope that in a near future I have time to work on migrate the can-maps to DefineMap
Justin Meyer
@justinbmeyer
Jul 25 2018 17:35
I'm wary of changing the behavior back b/c it's been like this for a year: https://github.com/canjs/can-map/releases/tag/v3.2.0
well, our goal is you shouldn't have to migrate them unless you are actively working w/ that code
can-map is still in 4.0 and 5.0
If I we set the behavior back, we could break other people's apps.
This is why I think some flag might be best
can.Map.legacyAttrBehavior = true
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:38
or an extra argument ...
Justin Meyer
@justinbmeyer
Jul 25 2018 17:38
or maybe even better ... on the prototype so it can be turned off on individual can-maps
can.Map.prototype.legacyAttrBehavior = true => set behavior for EVERY can.Map
And then for just one map type:
can.Map.extend({ legacyAttrBehavior: true}})
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 17:41
Ok, if this is implemented will this be available only on the latest can-map@4.x or it can be still be implemented for can-map@3.x
?
Justin Meyer
@justinbmeyer
Jul 25 2018 17:42
I'd probably do it for can-map@3
b/c we should NOT have broken this within 3
so it's most important to have some sort of fix there
when people upgrade from 3->4 ... it's probably not too difficult to use these workarounds I'm giving you
Justin Meyer
@justinbmeyer
Jul 25 2018 17:48
canjs/can-map#112
@ivospinheiro I'm going to see what I can do right now ...
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 18:31
Thanks @justinbmeyer!
I will go probably for the workaround for now
Ivo Pinheiro
@ivospinheiro
Jul 25 2018 21:36
Thanks @justinbmeyer for solving this issue so quickly
Will this be also available on can-map@4?
Because currently I'm upgrading the application to the latest can@3 but I hope to upgrade soon to can@4 and then can@5
Justin Meyer
@justinbmeyer
Jul 25 2018 22:13
Yeah, probably