Skip to content

Pass block info to extension methods #2102

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Pass block info to extension methods #2102

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca7977f
pass `blockInfo` to extension methods
Apr 8, 2019
94e4b59
add test for extension methods receiving `blockInfo`
Apr 9, 2019
ab84dd0
retrieve blockInfo from args when isDynamic is set
Apr 19, 2019
88a2d2a
add test for passing dynamic blockInfo to extension method
Apr 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/extension-support/extension-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,23 +391,22 @@ class ExtensionManager {
}
break;
default:
if (!blockInfo.opcode) {
throw new Error('Missing opcode for block');
}

blockInfo.func = blockInfo.func ? this._sanitizeID(blockInfo.func) : blockInfo.opcode;
if (blockInfo.opcode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for rearranging this conditional (from the previous structure of falsey check and early exit)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of silly... but this isn't legal JavaScript:

default:
    if (!blockInfo.opcode) {
        throw new Error('Missing opcode for block');
    }
    const funcName = "stuff"; // it's not legal to make a new `const` here

You can do it if you introduce a new scope with {} which sort of naturally led me to the arrangement proposed here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm okay. I think I'd prefer adding the new block of scope only because it's easier to read an early exit vs. keeping track of which block of code corresponds to which part of the if/else etc. Esp. since there are nested ifs involved here.

const funcName = blockInfo.func ? this._sanitizeID(blockInfo.func) : blockInfo.opcode;

// Avoid promise overhead if possible
if (dispatch._isRemoteService(serviceName)) {
blockInfo.func = dispatch.call.bind(dispatch, serviceName, blockInfo.func);
} else {
const serviceObject = dispatch.services[serviceName];
const func = serviceObject[blockInfo.func];
if (func) {
blockInfo.func = func.bind(serviceObject);
// Avoid promise latency unless necessary
if (dispatch._isRemoteService(serviceName)) {
blockInfo.func = (args, util) => dispatch.call(serviceName, funcName, args, util, blockInfo);
} else {
throw new Error(`Could not find extension block function called ${blockInfo.func}`);
const serviceObject = dispatch.services[serviceName];
if (!serviceObject[funcName]) {
// The function might show up later as a dynamic property of the service object
log.warn(`Could not find extension block function called ${funcName}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function exit after warning here? Below, the function RHS of the arrow function is just going to get bound to undefined(args, util, blockInfo) right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's looking up serviceObject[funcName] in the body of the (anonymous) function, it's possible that serviceObject[funcName] will become a valid function call between now and the first time it gets called. This could happen if the extension dynamically generates methods and monkey-patches itself, for example, especially if it needs to download some data first.

I don't expect this to be common but technically the way we make "remote" service calls doesn't even do this check at all, so I figured that if it's allowed there it should be allowed here. 🤷‍♂️

}
blockInfo.func = (args, util) => serviceObject[funcName](args, util, blockInfo);
}
} else {
throw new Error('Missing opcode for block');
}
break;
}
Expand Down
20 changes: 18 additions & 2 deletions test/integration/internal-extension.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const test = require('tap').test;
const Worker = require('tiny-worker');

const BlockType = require('../../src/extension-support/block-type');

const dispatch = require('../../src/dispatch/central-dispatch');
const VirtualMachine = require('../../src/virtual-machine');

Expand Down Expand Up @@ -33,8 +35,9 @@ class TestInternalExtension {
};
}

go () {
go (args, util, blockInfo) {
this.status.goCalled = true;
return blockInfo;
}

_buildAMenu () {
Expand Down Expand Up @@ -62,9 +65,22 @@ test('internal extension', t => {
t.type(func, 'function');

t.notOk(extension.status.goCalled);
func();
const goBlockInfo = func();
t.ok(extension.status.goCalled);

// The 'go' block returns its own blockInfo. Make sure it matches the expected info.
// Note that the extension parser fills in missing fields so there are more fields here than in `getInfo`.
const expectedBlockInfo = {
arguments: {},
blockAllThreads: false,
blockType: BlockType.COMMAND,
func: goBlockInfo.func, // Cheat since we don't have a good way to ensure we generate the same function
opcode: 'go',
terminal: false,
text: 'go'
};
t.deepEqual(goBlockInfo, expectedBlockInfo);

// There should be 2 menus - one is an array, one is the function to call.
t.equal(vm.runtime._blockInfo[0].menus.length, 2);
// First menu has 3 items.
Expand Down