Allow partial JSON for command callers but not receivers

[blink1073]

Alternative to #33, addresses #32 (comment).

Allows invokers of commands to pass in PartialReadonlyJSONObject values, but ensures that the command consumers receive ReadonlyJSONObject values (through API and by stripping undefined values).

[vidartf]

While this compiles, does this make sense? E.g. won't this allow errors to creep into the JS that were previously caught by the compiler?

execute(args) {
  for (let key in args) {
    if (args[key] !== null) {
      console.log(args[key].toString());
    }
  }
}

commands.execute('myid', { foo: undefined });

[blink1073]

We could strip out all args that are undefined

[vidartf]

We could strip out all args that are undefined

I think that would defeat the purpose of accepting partial types 😄

[blink1073]

I don't think so - we aren't claiming to use any undefined params for consumers.

[blink1073]

Notes:

• add methods to JSONExt to convert from Partial to regular JSON types, stripping out anything that is unknown
• use those methods whenever we are converting from one type to the other
• add a test to make sure we can pass a Partial object to a command, and that we can cast the args of the execute method to a Partial as well, and that any undefined values are stripped
• add tests for the utility methods

[blink1073]

It turned out that JSONExt.deepyCopy() already removed undefined values, so I used that instead.

[blink1073]

After this is merged, I'll cut a patch of commands to fix what was a breaking change in the last minor, and a minor release of widgets.

[vidartf]

Ci failure seems relevant.

[blink1073]

I addressed it in the last commit, does that look good to you?

[blink1073]

What the heck, it is working for me locally...

[vidartf]

It fails for me locally. Maybe a git clean -fdx?

[blink1073]

I couldn't find a way to allow PartialReadonlyJSONObject as the args type for the execute callback, even with a type cast, since we can't make PartialReadonlyJSONObject extend ReadonlyJSONObject. I think this is good to go now.

[blink1073]

@vidartf are we good to merge this?

[vidartf]

I've been going back and forth with myself whether I think this is a good idea. Adding a bunch of recursive copy operations just to satisfy backwards compatibility / typings seems like a bad trade to make. On the other hand, I don't want this to be left unresolved much longer.

[blink1073]

What if instead of deepCopy we use a different method that strips out any values that are undefined? It would still have to be recursive, though.

[telamonian]

Adding a bunch of recursive copy operations just to satisfy backwards compatibility / typings seems like a bad trade to make

That sounds scary. There's no limit on the depth of a json obj, so recursive anything could end up being arbitrarily expensive. Since this is part of Lumino, I think that in general we should favor performance over getting the typings perfect.

As a pragmatic matter, how frequently are calls actually made to eg execute? From what I can remember, commands are usually execute-ed only in direct response to user input (though some commands do execute other commands). Are there any existing situations in which execute gets called many times in quick succession?

Also, would the removeUndefined routine get removed in the next major release, or would this become a permanent feature of how commands are handled?

[blink1073]

It isn't just execute, menu calls things link isEnabled before every render. In theory you wouldn't be passing around deeply nested JSON as command args, but it is possible. I think it would be fair for removeUndefined to stick around.

Another alternative is to expose just a removeUndefined helper that folks can use on the object they're passing to execute().

[vidartf]

Just for clarity and summary: The main point of this work is that previous to #32, command implementors could rely on that if a key was present in the args, then the corresponding value was not undefined. #32 changed that, and this PR is being proposed as a fix to put us back into backwards compatibility.

The other alternative is to revert #32, but add a utility method removeUndefined/stripUndefined on JSONExt (unPartial?).

The last alternative is a more pragmatic "what is done is done", and just leave things as-is. Not sure about that one.

[vidartf]

What if instead of deepCopy we use a different method that strips out any values that are undefined? It would still have to be recursive, though.

It is likely to be a lot cheaper since it modifies in-place. I vote for this, given the alternatives. Then we can potentially relax this constraint for the next major version bump.

[blink1073]

Updated to add the stripUndefined function. We still have to traverse into arrays, since they may contain objects as value.