[WIP] TASK-1270, TASK-1271 Update Output cell mechanism #1214
Conversation
briehl
changed the title
|
Okay, some other work crept in here, but here's the main list of architectural changes. I think all the creep is below, too. The core target - update the viewer cell instantiation mechanism while leaving it backward-compatible - is still the same.
extra feature creep
|
| serialArr.push(serialize(upa)); | ||
| }); | ||
| return serialArr; | ||
| } |
There was a problem hiding this comment.
I never remember what's available...do we have map in our environment? If so, this could just be:
return upas.map( function(upa) { return serialize(upa) } );
Not actually necessary to fix, just a little prettier. Even better when we could have
return upas.map( upa => serialize(upa) )
There was a problem hiding this comment.
Yeah, map's available. Not sure why I never think of it. I'll switch that out.
| serialSt[key] = serializeAll(upas[key]); | ||
| }); | ||
| return serialSt; | ||
| } |
There was a problem hiding this comment.
Same comment as above, could be:
return upas.reduce( function(acc, key) { acc[key] = serializeAll(upas[key]); return acc }, {} );
...but doesn't need to be.
| Object.keys(upas).forEach(function(key) { | ||
| deserialSt[key] = deserializeAll(upas[key]); | ||
| }); | ||
| return deserialSt; |
There was a problem hiding this comment.
Same commentary on 145...156 - could be a map and a reduce, but not urgent. It's such a big pull I feel obligated to add notes. :-)
| // handle where options.widget == null. | ||
| options.widget = options.widget || this.options.widget; | ||
| // handle where options.widget == 'null' string. I know. It happens. | ||
| options.widget = options.widget === 'null' ? this.options.widget : options.widget; |
There was a problem hiding this comment.
Before coming to CSHL, I worked at Edison Schools and we imported school data from their SIS systems into our servers every night. One night, it broke at a school. I dug into it. Turns out they had a new kid whose name was "Brandon Null", and our importer failed because it said that his last name was a required field.
So our CSR called up the school and said he was missing his last name. They replied, "What are you talking about? His name is Brandon Null. That didn't go through? Why not?" Poor kid. He's gonna have all sorts of problems in life.
The quick fix was when I realized that the importer only failed on insert, not update - so I just change his name to "Brandon NotNull", imported it, and then went into the database to manually change back to "Brandon Null". After that, his record would update without issue.
We never encountered that specific bug again during my tenure.
There was a problem hiding this comment.
Ha! Well, all over the place in various app specs, there's the JSON value null mixed in with the string "null".
Makes for some fun trickiness there, too.
There was a problem hiding this comment.
In the famous words of Donald Knuth, beware of bugs in the above code; I have only proved it correct, not tried it.
i.e., so far I've just read through it for syntactic/logic things and not tried executing anything. I contributed a couple of minor syntax suggestions (which can be ignored) and an anecdote. But otherwise it looks fine to me, keeping in mind that I've just eyeballed it atm.
|
This is great so far. Thanks for the feedback! Added the suggestions (and twiddled some tests around, too). I keep forgetting that reduce is just built in to Javascript... |
|
Ok, after some conversation, I think this'll get frozen for now and merged next week, after staging up a new release. |
The first stage here is to update the output/viewer cells for some consistency. Before merging, these cells should (see checklist below):
(see here for some details on requirements, etc.: https://docs.google.com/document/d/1Ie7qb8rEvnxXi_jrgYu2TIWmhNIuHjqaYA4J3AFCof8/edit )
Note that this work might get separated into multiple PRs, I'm just making a checklist here of the broad strokes.
These will be worked on in another PR.