blockly-samples icon indicating copy to clipboard operation
blockly-samples copied to clipboard

feat: add a migration script to fix imports

Open BeksOmega opened this issue 3 years ago • 3 comments

Description

Adds a new migration script npx @blockly/migrate fix-imports that will automatically add new imports, and rename references to the old location of the import on the blockly namespace tree to now reference the import.

Testing

Created some manual test data and ran the migration script against it. Everything seems to look good!

Additional Info

This isn't how I would ideally like the system to function. In particular:

  1. I would like the database to be separate from the script, and live in core.
  2. I don't want to have to specify the import string explicitly. I would rather be able to specify if it is named, default, or namespace.

But since we want to get this out before the release, I don't think there's any reason to add that extra complexity.

BeksOmega avatar Sep 27 '22 21:09 BeksOmega

It didn't occur to me when we discussed this yesterday, but any file that needs this migration should already contain an import 'blockly/javascript'; statement (or the like).

  • Files that rely on Blockly and e.g. Blockly.JavaScript having already been loaded via a <script> tag won't have those imports and don't need this migration.
  • Files that do have the import need this migration but the developer has already identified where the new import should go, since it should replace the existing one.

I don't think this is true for the javascript generator or the library blocks (although it is true for the other generators). You can access Blockly.JavaScript when you do import Blockly from 'blockly'; or import Blockly from 'blockly/core';. So in those cases references would need to be renamed, and the import would need to be added.

Or at least that's my understanding anyway.

BeksOmega avatar Sep 28 '22 20:09 BeksOmega

I don't think this is true for the javascript generator or the library blocks (although it is true for the other generators). You can access Blockly.JavaScript when you do import Blockly from 'blockly'; or import Blockly from 'blockly/core';. So in those cases references would need to be renamed, and the import would need to be added.

Or at least that's my understanding anyway.

Oh yes, I forgot about our "some batteries included" default. Hmm… 🤔

I guess the best we can do is to try to add the new imports after the Blockly import/require). (We can definitely at least check check for the existence of such to figure out if we need to apply the migration, anyway.)

cpcallen avatar Sep 29 '22 14:09 cpcallen

I think this is ready for another look @cpcallen

I really hate the organization of this (i.e. using if statements everywhere to change the behavior based on the type of import) but I don't know that reorganizing it rn is worth it.

BeksOmega avatar Sep 29 '22 17:09 BeksOmega

@cpcallen Should be good for another look =)

BeksOmega avatar Oct 03 '22 18:10 BeksOmega