-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add ability to convert old block factory json to new #2304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Mostly just type nits.
newBlock.inputs = { | ||
CHECK: { | ||
shadow: CONNECTION_CHECK_SHADOW, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be defined regardless of whether a TYPE
input exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would create data for "CHECK" inputs on the dummy and end-row blocks, which don't have connection checks because they don't have connections. I'm not sure if the serializer would throw an error if it reads data for an input that doesn't exist on the block, but even if it doesn't, we don't need it.
* @param oldBlock JSON for the "field_foo" block as saved from old tool. | ||
* @returns JSON that should be used for the replacement field block. | ||
*/ | ||
function convertField(oldBlock: any): object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing an if(!oldBlock)
check? Would probably be helpful if this was at least object | null
instead of any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, before calling this method I was always making sure there is a field block. But I was also doing that for most of the other methods too, so I also removed it from them so I don't have to accept/return null.
} | ||
let connectionCheck = oldName.substring(5); | ||
switch (connectionCheck) { | ||
case 'null': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So null
continues to be lower case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and I added a comment so you can tell it's intentional (the reason the others are capitalized is the dropdown value matches what blockly uses internally for those types; for null
connection checks, it's just set to null
in the json block definition and not 'null'
but we check for that exception)
* feat: add ability to convert old block factory json to new * chore: format and use constant * fix: use better typings, minor refactoring
* feat: add basic infrastructure of developer-tools * chore: add eof newline where missing * fix: formatting, license, css attributes * feat: add block factory blocks and toolbox (#2155) * feat: add block factory blocks and toolbox * fix: pr comments * fix: change type to connection check * fix: formatting and comments * chore: lint and format developer-tools (#2185) * chore: autofix lint * chore: manually fix most of the lint * chore: format * feat: add basic saving and loading and start block state (#2187) * feat: add json definition generator (#2188) * feat: add javascript definition generators for blocks (#2196) * feat: add javascript definition generators for blocks * chore: format * fix: switch statement style Co-authored-by: Christopher Allen <[email protected]> * fix: update to latest blockly to use JavascriptGenerator class * fix: fix img dropdown option --------- Co-authored-by: Christopher Allen <[email protected]> * feat: add controllers and models to switch between definitions (#2219) * feat: add code header generation for imports and script tags (#2286) * feat: add save, load, and delete functionality to dev-tools (#2285) * feat: add save, load, and delete functionality to dev-tools * chore: format * chore: update load name * feat: add generator stubs to block factory (#2295) * feat: add generator stub generator and output * feat: add generator headers * chore: add more tsdoc * chore: minor refactoring of template strings * feat: save block factory settings (#2297) * feat: save block factory settings * chore: const to named functions * feat: add ability to convert old block factory json to new (#2304) * feat: add ability to convert old block factory json to new * chore: format and use constant * fix: use better typings, minor refactoring * feat: add shadow blocks for connection checks and real colour block (#2307) * feat: add file upload for block factory (#2320) * feat: add file upload for block factory * chore: fix questionable html formatting * chore: rename and comments * feat: add angle and colour fields to block factory (#2325) * feat: add angle and colour fields to block factory * fix: call register fields in script header * feat: support uploading file from old block factory (#2336) * feat: support uploading file from old block factory * feat: support multiple file input, minor pr fixes * feat: update to blockly v11 & improve style (#2388) * fix: styling * fix: changes for v11 * fix: set max height in narrow mode * fix: min height of code divs * chore: format * chore: remove log * feat: use a js legal name for the block in code output (#2392) * feat: use a js legal name for the block in code output * fix: legal js name probably * feat: add help button and favicon (#2396) * feat: include developer-tools when publishing to gh-pages (#2395) --------- Co-authored-by: Christopher Allen <[email protected]>
The basics
The details
Resolves
Works on #2291
Proposed Changes
factory_base
block from the old block factory on app engine into JSON that can be loaded into the new tool, given the changes to block definitions that were madeReason for Changes
There are two options for fixing this:
Manual JSON editing
Pros: Relatively straightforward to edit JSON (as opposed to XML); pretty obvious exactly what changed between versions
Cons: Mildly complicated to explain what's happening (but hopefully I left good comments)
Alternate block definitions
meaning define blocks that match the old ones, and just not include them in the toolbox. e.g. define a
input_value
block.Pros: This is the approach we recommend people use if they make breaking changes to their blocks.
Cons: The blocks have the same name in both tools, so I'd either have to rename some of them or manually edit the json anyway to change their names; I'd have to copy & paste the generators for each of the changed blocks and very slightly adjust them to deal with the different field names, etc. so it'd be difficult to tell what changed without diffing the generators (and there are multiple generators for each block); users importing old blocks will be forced to continue using the old blocks and can't use the new ones which are a bit easier to use
Both approaches have cons, but creating alternate block definitions had more cons so I went with the JSON approach. Not as much needed to be adjusted as I feared.
Test Coverage
Made several very complicated blocks in the old tool that exercised every feature of block creation, then made sure the json could be loaded into the new tool and the block looked the exact same.
Documentation
Additional Information
This is only the actual converter code in this PR. I'm still working on the file upload part of the UI to allow you to actually easily import json, so I'll do that in a follow-up PR.