block-shareable-procedures: Spurious callers added to flyout

[laurensvalk]

Category

• Plugins

Component

block-shareable-procedures

Describe the bug

When creating new procedures, spurious callers are added to the flyout.

Oddly enough, this appears to only happen when installing with yarn. With npm, all seems to work fine. This took me a while to figure out 😄

update-2023-08-12_09.34.26.mp4

To Reproduce

I was able to reproduce this on the latest master build and the 2023-q3-eq-release.

No code changes are needed; this can be reproduced directly in this repo with its example project.

cd blockly-samples/plugins/block-shareable-procedures

rm -rf node_modules
rm package-lock.json

yarn add typescript --dev # Without this, install will pass but on start says Typescript is missing
yarn install
yarn start

All seems to work fine, until you add procedures, as shown in the video above.

Expected behavior

No spurious callers added. This video was made when installing with npm and everything works as expected:

update-2023-08-12_09.32.27.mp4

Additional context

I originally arrived at this problem when trying to make my own customized flyout for the procedure category, but this turns out unrelated.

[maribethb]

Hi @laurensvalk, is it possible you have two different copies of Blockly installed when you're installing with yarn? I don't know why exactly that would cause this problem, but it is something that we've seen happen before if you install plugins in an unusual state.

In particular, for your repro instructions, we don't actually support installing plugins that way. Because blockly-samples is a monorepo, we use lerna to crosslink the plugins that depend on each other. So when running plugins from within blockly-samples, it's important that you only run npm install at the root level of samples so that lerna bootstrap is run and handles installing local dependencies. Your yarn instructions don't do that, so I suspect that you might be getting multiple copies of Blockly installed which somehow causes this.

In npm, you can check by running npm ls blockly and determining how many times blockly is installed. Not sure if there is an equivalent in yarn.

If you are able to reproduce this outside of the blockly-samples monorepo structure (e.g. if you can install the plugin in a separate repo and send us a link) that would be helpful and would point to there being a real issue here. Thanks!

[laurensvalk]

Thank you. As a workaround, I have reverted to Blockly 10.0.2 where this doesn't seem to happen.

I will try to reproduce on a fresh separate copy of blockly-samples/sample-app-ts where I originally started from.

---

On a related note, it could be worth updating the usage instructions.

The word from seems to be missing, but more importantly, registerProcedureSerializer as well. This was causing all sorts of other issues 😄

[laurensvalk]

I will try to reproduce on a fresh separate copy of blockly-samples/sample-app-ts where I originally started from.

This reproduces it as well. And unlike what I mentioned above, it happens with npm as well, so that comment is probably unrelated. To reproduce:

• Clone blockly-samples from master. I used f8cbacf.
• Copy examples/sample-app-ts to a folder outside this repo, just in case.
• Clean up old install:

rm -rf node_modules/
rm package-lock.json

• Apply this diff:

diff --git a/package.json b/package.json
index 218caf6..a0c8aff 100644
--- a/package.json
+++ b/package.json
@@ -26,6 +26,7 @@
     "webpack-dev-server": "^4.11.1"
   },
   "dependencies": {
-    "blockly": "^10.0.2"
+    "@blockly/block-shareable-procedures": "^3.0.2",
+    "blockly": "^10.1.2"
   }
 }
diff --git a/src/index.ts b/src/index.ts
index 12fc83a..77ec250 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -5,6 +5,7 @@
  */
 
 import * as Blockly from 'blockly';
+import {blocks as procedureBlocks, unregisterProcedureBlocks, registerProcedureSerializer} from '@blockly/block-shareable-procedures';
 import {blocks} from './blocks/text';
 import {generator} from './generators/javascript';
 import {javascriptGenerator} from 'blockly/javascript';
@@ -16,6 +17,10 @@ import './index.css';
 Blockly.common.defineBlocks(blocks);
 Object.assign(javascriptGenerator, generator);
 
+unregisterProcedureBlocks();
+Blockly.common.defineBlocks(procedureBlocks);
+registerProcedureSerializer();
+
 // Set up UI elements and inject Blockly
 const codeDiv = document.getElementById('generatedCode')?.firstChild;
 const outputDiv = document.getElementById('output');
diff --git a/tsconfig.json b/tsconfig.json
index 058436c..075ba53 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -7,6 +7,6 @@
         "module": "ES2015",
         "moduleResolution": "node",
         "target": "ES2015",
-        "strict": true,
+        "strict": false, // I only did this to make the import {...} from work. May not be related. Could still be nice to use one ts-based plugin in the sample-app-ts to exemplify the recommended approach.
     },
 }

• Install and start

npm install
npm start

• Drag two new functions into the workspace.
• Flyout now contains 4 callers, similar to video above.

[BeksOmega]

Hm so whatever this is it's a bug in core. I'm testing using the samples playground. If I have v10 of Blockly, no spurious callers. If I'm using v10.1, then spurious callers.

[BeksOmega]

So this was caused by the change to make insertion markers use the JSON serialization system. Before the blocks would be set as insertion markers /before/ their extra state was loaded, but now it is happening /after/. So our checks to see if the block is an insertion marker aren't working.

This is fixed in the 2023-q3-eoq branch because of #1863 which checks for fullSerialization instead of isInsertionMarker().

We /may/ have broken other blocks which use backing data models by making the change to use the JSON serialization system. But we don't know of any other blocks besides the shareable procedures ones that do this. So I don't think it requires a path release.

Not sure what to do about the state of the shareable procedure blocks though. They only way to fix them is either revert the JSON serialization in core and release a patch, or wait until the EOQ release.

@maribethb

[maribethb]

Before the blocks would be set as insertion markers /before/ their extra state was loaded, but now it is happening /after/.

That seems like a breaking change for any mutators that care about whether their block is an insertion marker.

Rachel & I think we should rollback the change to make insertion markers use JSON serialization. We can do that again the EOQ release which also contains the change to have the doFullSerialization parameter. But we should consider whether that insertion marker change is potentially actually a breaking change, for more blocks than just those using data models.

[rachel-fenichel]

The change that switched insertion markers to use JSON serialization is google/blockly#7364

[rachel-fenichel]

And google/blockly#7384 builds on google/blockly#7364 to fix a bug.

[BeksOmega]

@rachel-fenichel Can this be closed since we did the rollback?

[rachel-fenichel]

Yes, although we will also need to verify it when rolling forward.