E16n serialization (slightly more conservative version) #2279
Conversation
cwillisf
force-pushed
the
e16n-serialization-conservative
branch
from
51b840e to
5920fad
Compare
There was a problem hiding this comment.
I did a preliminary review of this code. I tried to be pretty thorough in this pass, but did not look carefully at the documentation or the new _prepareSerializationInfo function yet. I think there's a lot to talk about here, so it might be good to discuss some of this in person.
- Fix a lint error due to an unused constant. - Don't return an extended opcode from `deserializeVariable` (more changes needed here). - If `refreshBlocks` encounters an extension ID mismatch, keep the old ID instead of switching to the new one. - Rename `decodeBlock` to `getExtensionAndOpcode` and clarify its purpose. In particular, distinguish it from `getExtensionIdForOpcode`.
cwillisf
force-pushed
the
e16n-serialization-conservative
branch
from
37b9f9a to
695a8f7
Compare
| const result = { | ||
| opcode: 'variable', | ||
| fields: { | ||
| VARIABLE: { |
There was a problem hiding this comment.
I don't think we need this field descriptor here. This block should only need the mutation.
| // something that looks like the block's entry in `getInfo()`. If we do either of those things we'll need to | ||
| // decide how we want to handle the block's (x,y) position and `topLevel` flag. | ||
| const result = { | ||
| opcode: 'variable', |
There was a problem hiding this comment.
This opcode is incorrect, the top level block opcode is the "extended" opcode.
There was a problem hiding this comment.
I was trying to avoid having the extension every mention the extended opcode, since in theory we might sometimes post-process the extension ID so that it isn't what the extension author expects. Instead I fix up the ID after the fact, in the function which calls this one.
| parent: null, | ||
| shadow: false, | ||
| mutation: { | ||
| blockInfo: { |
There was a problem hiding this comment.
Similar to the comment above, I think it would be better for this function to just return this blockInfo object and have the rest of this top-level object constructed in either a helper function or by the sb3 deserialize code itself.
| serializeVariable (block, target) { | ||
| const variableName = block.mutation.blockInfo.text; |
There was a problem hiding this comment.
Probably this function should just take blockInfo as an argument, and then some helper function outside of the extension definition should handle high level block things like setting the x and y.
There was a problem hiding this comment.
The problem is that variables have their own custom way of serializing x and y, so it kind of needs to be here. That's also why the custom deserialize function needs to return the whole block instead of just the blockInfo guts :/
| if (customDeserialize) { | ||
| block = customDeserialize(block); | ||
| blocks[blockId] = block; // customDeserialize might have made a new object; make sure to keep it! | ||
| // the deserialize function should return opcode 'foo' and we'll fix it to 'ext_foo' |
There was a problem hiding this comment.
Related to the comment earlier in this PR, if we're doing fix-up here anyways of what the customDeserialize function returns, I think we might as well construct the block JSON here instead of the customDeserialize function and have that function just return the blockInfo.
|
Unassigning myself from this PR since we discussed offline (a few weeks ago) that maybe custom serialization in the extension spec is not the right approach since we only want it in this specific case of the core variables extension. We can handle this special case in the "backend" (e.g. vm runtime or extension manager) instead of making this a developer facing feature. We also discussed removing this from the immediate scope of this "first phase" of the e16n project (wrapping up by the end of this year). |
does this mean custom variable types are out of the question for developer extensions? not that I actually know what's going on in this code, nor that it'd mean that wouldn't be supported ever, but this was one of the most exciting things about extensions IMO (localstorage-based variables, etc). |
This is the same as #2245 but does not move the Extension Manager into the Runtime. Instead, only the information about currently-loaded extensions is moved to the Runtime.