hmm I'm not a big fan of mutating function arguments, even when documented it's easy to miss
2018-01-01 00133, 2018
naiveai
arthelon[m]: agreed, but since entityEditorMarkup is the only one with acess to `store` (and ideally should be the only one), I don't see any other way
2018-01-01 00148, 2018
naiveai
other than perhaps returning the store.getState(), but that seems less clean
2018-01-01 00102, 2018
arthelon[m]
you can change the final expression to "{markup, props: {...props, initialState: store.getState()}};"
2018-01-01 00117, 2018
naiveai
arthelon[m]: ah, that makes sense.
2018-01-01 00128, 2018
naiveai
arthelon[m]: that won't break caller code either!
2018-01-01 00143, 2018
naiveai
arthelon[m]: am making a commit that does this
2018-01-01 00145, 2018
arthelon[m]
yeah, I'll leave that snippet in my review
2018-01-01 00148, 2018
Nyanko-sensei has quit
2018-01-01 00116, 2018
MajorLurker joined the channel
2018-01-01 00121, 2018
naiveai
arthelon[m]: should I use Object.assign instead?
2018-01-01 00143, 2018
arthelon[m]
Up to you. Both constructs get transpiled with babel anyways. Personally I prefer using spread though.
2018-01-01 00127, 2018
Slurpee joined the channel
2018-01-01 00106, 2018
arthelon[m]
I think the props mutation was the only issue I identified.
2018-01-01 00151, 2018
naiveai
kewl, am pushing in a minute
2018-01-01 00118, 2018
arthelon[m]
I'm wary of writing functions with more than 3 arguments, probably means that something can be simplified, but I don't see a better way at the moment.
2018-01-01 00104, 2018
arthelon[m]
LordSputnik: I'll leave it up to you to approve naiveai's gci task
2018-01-01 00128, 2018
naiveai
arthelon[m]: putting entityType and action in req seem to be the most obvious changes
2018-01-01 00141, 2018
naiveai
arthelon[m]: might figure out a way to do them in a different PR after gci
2018-01-01 00100, 2018
naiveai
arthelon[m]: for consistency I'm leaning towards Object.assign
2018-01-01 00129, 2018
arthelon[m]
alright
2018-01-01 00130, 2018
arthelon[m]
sure
2018-01-01 00122, 2018
Ashish joined the channel
2018-01-01 00157, 2018
naiveai
arthelon[m]: done!
2018-01-01 00141, 2018
arthelon[m]
if the initialState field exists on props, the initialState of the object literal you passed in as the first argument will be overriden
2018-01-01 00148, 2018
arthelon[m]
You should swap their places
2018-01-01 00133, 2018
arthelon[m]
make sure you add an empty object literal as the first argument though otherwise 'props' will be mutated by Object.assign
(essentially it's copying the properties of the objects in the arguments to the first argument in a sequence)
2018-01-01 00122, 2018
KassOtsimine
great now I read "man, i should probably try replacing coveralls with codecov sometime"
2018-01-01 00122, 2018
KassOtsimine
as "man, i should probably try replacing overalls with corderoy sometime"
2018-01-01 00124, 2018
KassOtsimine
>_<
2018-01-01 00150, 2018
KassOtsimine
srsly, brain
2018-01-01 00119, 2018
drsaunders joined the channel
2018-01-01 00129, 2018
LordSputnik
KassOtsimine: Possibly also a good choice ;)
2018-01-01 00104, 2018
KassOtsimine
:D
2018-01-01 00118, 2018
naiveai
LordSputnik: gimme a sec
2018-01-01 00135, 2018
naiveai
will fix
2018-01-01 00111, 2018
naiveai
LordSputnik: should I do a git revert --hard HEAD~1?
2018-01-01 00127, 2018
naiveai
it'll require a force push but a revert commit would clutter the history
2018-01-01 00128, 2018
LordSputnik
naiveai: yeah can do, then just force push to your repo
2018-01-01 00104, 2018
LordSputnik
My force push policy is a bit looser than Leftmost's - as long as the branch is unlikely to have been pulled since you made the original commit, I'm fine with it
2018-01-01 00113, 2018
naiveai
LordSputnik: done.
2018-01-01 00104, 2018
naiveai
now only 24 commits :(
2018-01-01 00115, 2018
naiveai
suggest something so I can make it 25 :D
2018-01-01 00144, 2018
dragonzeron has quit
2018-01-01 00155, 2018
naiveai
LordSputnik: wait, bit slow today, should I do Object.assign({}, {blah blah}, additionalProps) in getEntityProps
2018-01-01 00108, 2018
LordSputnik
Err let me see
2018-01-01 00134, 2018
arthelon[m]
if you want additionalProps to override then yes
2018-01-01 00156, 2018
naiveai
oof
2018-01-01 00118, 2018
LordSputnik
naiveai: I think it's fine as is
2018-01-01 00118, 2018
naiveai
what would the commit message for that be?
2018-01-01 00136, 2018
naiveai
LordSputnik: now I am confused.
2018-01-01 00141, 2018
naiveai
lemme check the docs again
2018-01-01 00142, 2018
LordSputnik
Object.assign({}, {a: 1}) is the same as Object.assign({a: 1})
2018-01-01 00111, 2018
LordSputnik
You already do Object.assign({blah blah}, additionalProps) so gain nothing by adding an extra empty object on the front
2018-01-01 00128, 2018
naiveai
LordSputnik: ya but arthelon[m] seems to be implying that Object.assign({}, {a: 1}, {a: 2}) would be the correct way to override
2018-01-01 00133, 2018
naiveai
LordSputnik: MDN uses Object.assign({}, obj) to clone `obj`
If props has an initialState key, the resulting object will have initialState= props.initialState with Object.assign({initialState: ...}, props)
2018-01-01 00125, 2018
naiveai
yes
2018-01-01 00146, 2018
LordSputnik
But in the original context, the last thing to happen to props was that initialState got set, so you want to do Object.assign({}, props, {initialState: ...})
2018-01-01 00149, 2018
SothoTalKer
props to you :D
2018-01-01 00111, 2018
LordSputnik
Which will create a new object, update it with the data from props, and then update the initialState
