From bc2824dfdc17a0fa5c9e5c4e590ac4300265c8d8 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 19 Mar 2019 15:31:30 -0400 Subject: [PATCH 1/7] Add an example core blocks category using the extension spec. --- src/blocks/scratch3_core_example.js | 44 +++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 src/blocks/scratch3_core_example.js diff --git a/src/blocks/scratch3_core_example.js b/src/blocks/scratch3_core_example.js new file mode 100644 index 000000000..b53d0b111 --- /dev/null +++ b/src/blocks/scratch3_core_example.js @@ -0,0 +1,44 @@ +const BlockType = require('../extension-support/block-type'); + +/** + * An example core block implemented using the extension spec. + * This is not loaded as part of the core blocks in the VM but it is provided + * and used as part of tests. + */ +class Scratch3CoreExample { + constructor (runtime) { + /** + * The runtime instantiating this block package. + * @type {Runtime} + */ + this.runtime = runtime; + } + + /** + * @returns {object} metadata for this extension and its blocks. + */ + getInfo () { + return { + id: 'coreExample', + name: 'CoreEx', // This string does not need to be translated as this extension is only used as an example. + blocks: [ + { + opcode: 'exampleOpcode', + blockType: BlockType.REPORTER, + text: 'example block' + } + ] + }; + } + + /** + * Example opcode just returns the name of the stage target. + * @returns {string} The name of the first target in the project. + */ + exampleOpcode () { + return this.runtime.getTargetForStage().getName(); + } + +} + +module.exports = Scratch3CoreExample; From 0e710ba3d9a82876535d3aa633bf43db7c4f6d71 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 20 Mar 2019 14:45:21 -0400 Subject: [PATCH 2/7] Allow loading extensions synchronously. Add example extension to list of known internal extensions. --- src/dispatch/central-dispatch.js | 38 +++++++++++++++-- src/extension-support/extension-manager.js | 48 ++++++++++++++++++---- src/virtual-machine.js | 13 +++--- 3 files changed, 79 insertions(+), 20 deletions(-) diff --git a/src/dispatch/central-dispatch.js b/src/dispatch/central-dispatch.js index a53e50d79..00c6ea77e 100644 --- a/src/dispatch/central-dispatch.js +++ b/src/dispatch/central-dispatch.js @@ -35,6 +35,39 @@ class CentralDispatch extends SharedDispatch { this.workers = []; } + /** + * Synchronously call a particular method on a particular service provided locally. + * Calling this function on a remote service will fail. + * @param {string} service - the name of the service. + * @param {string} method - the name of the method. + * @param {*} [args] - the arguments to be copied to the method, if any. + * @returns {*} - the return value of the service method. + */ + callSync (service, method, ...args) { + const {provider, isRemote} = this._getServiceProvider(service); + if (provider) { + if (isRemote) { + log.error("Cannot use 'callSync' on a remote service provider."); + return; + } + + return provider[method].apply(provider, args); + } + } + + /** + * Synchronously set a local object as the global provider of the specified service. + * WARNING: Any method on the provider can be called from any worker within the dispatch system. + * @param {string} service - a globally unique string identifying this service. Examples: 'vm', 'gui', 'extension9'. + * @param {object} provider - a local object which provides this service. + */ + setServiceSync (service, provider) { + if (this.services.hasOwnProperty(service)) { + log.warn(`Central dispatch replacing existing service provider for ${service}`); + } + this.services[service] = provider; + } + /** * Set a local object as the global provider of the specified service. * WARNING: Any method on the provider can be called from any worker within the dispatch system. @@ -45,10 +78,7 @@ class CentralDispatch extends SharedDispatch { setService (service, provider) { /** Return a promise for consistency with {@link WorkerDispatch#setService} */ try { - if (this.services.hasOwnProperty(service)) { - log.warn(`Central dispatch replacing existing service provider for ${service}`); - } - this.services[service] = provider; + this.setServiceSync(service, provider); return Promise.resolve(); } catch (e) { return Promise.reject(e); diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index 4d4e2d283..b5d1c55b0 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -9,6 +9,10 @@ const BlockType = require('./block-type'); // TODO: change extension spec so that library info, including extension ID, can be collected through static methods const builtinExtensions = { + // This is an example that isn't loaded with the other core blocks, + // but serves as a reference for loading core blocks as extensions. + coreExample: require('../blocks/scratch3_core_example'), + // These are the non-core built-in extensions. pen: () => require('../extensions/scratch3_pen'), wedo2: () => require('../extensions/scratch3_wedo2'), music: () => require('../extensions/scratch3_music'), @@ -108,6 +112,27 @@ class ExtensionManager { return this._loadedExtensions.has(extensionID); } + /** + * Synchronously load an internal extension (core or non-core) by ID. This call will + * fail if the provided id is not does not match an internal extension. + * @param {string} extensionId - the ID of an internal extension + */ + loadExtensionIdSync (extensionId) { + if (builtinExtensions.hasOwnProperty(extensionId)) { + /** @TODO dupe handling for non-builtin extensions. See commit 670e51d33580e8a2e852b3b038bb3afc282f81b9 */ + if (this.isExtensionLoaded(extensionId)) { + const message = `Rejecting attempt to load a second extension with ID ${extensionId}`; + log.warn(message); + return; // TODO Do we want to throw an error here? + } + + const extension = builtinExtensions[extensionId]; + const extensionInstance = new extension(this.runtime); + const serviceName = this._registerInternalExtension(extensionInstance); + this._loadedExtensions.set(extensionId, serviceName); + } + } + /** * Load an extension by URL or internal extension ID * @param {string} extensionURL - the URL for the extension to load OR the ID of an internal extension @@ -124,7 +149,7 @@ class ExtensionManager { const extension = builtinExtensions[extensionURL](); const extensionInstance = new extension(this.runtime); - return this._registerInternalExtension(extensionInstance).then(serviceName => { + return Promise.resolve(this._registerInternalExtension(extensionInstance)).then(serviceName => { this._loadedExtensions.set(extensionURL, serviceName); }); } @@ -150,7 +175,7 @@ class ExtensionManager { dispatch.call('runtime', '_refreshExtensionPrimitives', info); }) .catch(e => { - log.error(`Failed to refresh buildtin extension primitives: ${JSON.stringify(e)}`); + log.error(`Failed to refresh built-in extension primitives: ${JSON.stringify(e)}`); }) ); return Promise.all(allPromises); @@ -163,6 +188,15 @@ class ExtensionManager { return [id, workerInfo.extensionURL]; } + /** + * Synchronously collect extension metadata from the specified service and begin the extension registration process. + * @param {string} serviceName - the name of the service hosting the extension. + */ + registerExtensionServiceSync (serviceName) { + const info = dispatch.callSync(serviceName, 'getInfo'); + this._registerExtensionInfo(serviceName, info); + } + /** * Collect extension metadata from the specified service and begin the extension registration process. * @param {string} serviceName - the name of the service hosting the extension. @@ -191,17 +225,15 @@ class ExtensionManager { /** * Register an internal (non-Worker) extension object * @param {object} extensionObject - the extension object to register - * @returns {Promise} resolved once the extension is fully registered or rejected on failure + * @returns {string} The name of the registered extension service */ _registerInternalExtension (extensionObject) { const extensionInfo = extensionObject.getInfo(); const fakeWorkerId = this.nextExtensionWorker++; const serviceName = `extension_${fakeWorkerId}_${extensionInfo.id}`; - return dispatch.setService(serviceName, extensionObject) - .then(() => { - dispatch.call('extensions', 'registerExtensionService', serviceName); - return serviceName; - }); + dispatch.setServiceSync(serviceName, extensionObject); + dispatch.callSync('extensions', 'registerExtensionServiceSync', serviceName); + return serviceName; } /** diff --git a/src/virtual-machine.js b/src/virtual-machine.js index daf9df544..82375c429 100644 --- a/src/virtual-machine.js +++ b/src/virtual-machine.js @@ -151,6 +151,11 @@ class VirtualMachine extends EventEmitter { this.extensionManager = new ExtensionManager(this.runtime); + // Load core extensions + for (const id of CORE_EXTENSIONS) { + this.extensionManager.loadExtensionIdSync(id); + } + this.blockListener = this.blockListener.bind(this); this.flyoutBlockListener = this.flyoutBlockListener.bind(this); this.monitorBlockListener = this.monitorBlockListener.bind(this); @@ -493,14 +498,6 @@ class VirtualMachine extends EventEmitter { installTargets (targets, extensions, wholeProject) { const extensionPromises = []; - if (wholeProject) { - CORE_EXTENSIONS.forEach(extensionID => { - if (!this.extensionManager.isExtensionLoaded(extensionID)) { - extensionPromises.push(this.extensionManager.loadExtensionURL(extensionID)); - } - }); - } - extensions.extensionIDs.forEach(extensionID => { if (!this.extensionManager.isExtensionLoaded(extensionID)) { const extensionURL = extensions.extensionURLs.get(extensionID) || extensionID; From 30c9b7fd849bb4e7e19e7b655f63d70f5f00db1c Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Fri, 22 Mar 2019 12:13:37 -0400 Subject: [PATCH 3/7] Add tests and update core example to handle stage being undefined. --- src/blocks/scratch3_core_example.js | 3 +- test/integration/internal-extension.js | 61 +++++++++++++++++++------- test/unit/dispatch.js | 21 +++++++++ 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/blocks/scratch3_core_example.js b/src/blocks/scratch3_core_example.js index b53d0b111..7781bf25a 100644 --- a/src/blocks/scratch3_core_example.js +++ b/src/blocks/scratch3_core_example.js @@ -36,7 +36,8 @@ class Scratch3CoreExample { * @returns {string} The name of the first target in the project. */ exampleOpcode () { - return this.runtime.getTargetForStage().getName(); + const stage = this.runtime.getTargetForStage(); + return stage ? stage.getName() : 'no stage yet'; } } diff --git a/test/integration/internal-extension.js b/test/integration/internal-extension.js index 570fce3cb..476981c05 100644 --- a/test/integration/internal-extension.js +++ b/test/integration/internal-extension.js @@ -4,6 +4,9 @@ const Worker = require('tiny-worker'); const dispatch = require('../../src/dispatch/central-dispatch'); const VirtualMachine = require('../../src/virtual-machine'); +const Sprite = require('../../src/sprites/sprite'); +const RenderedTarget = require('../../src/sprites/rendered-target'); + // By default Central Dispatch works with the Worker class built into the browser. Tell it to use TinyWorker instead. dispatch.workerClass = Worker; @@ -52,23 +55,49 @@ test('internal extension', t => { t.ok(extension.status.constructorCalled); t.notOk(extension.status.getInfoCalled); - return vm.extensionManager._registerInternalExtension(extension).then(() => { - t.ok(extension.status.getInfoCalled); + vm.extensionManager._registerInternalExtension(extension); + t.ok(extension.status.getInfoCalled); - const func = vm.runtime.getOpcodeFunction('testInternalExtension_go'); - t.type(func, 'function'); + const func = vm.runtime.getOpcodeFunction('testInternalExtension_go'); + t.type(func, 'function'); - t.notOk(extension.status.goCalled); - func(); - t.ok(extension.status.goCalled); + t.notOk(extension.status.goCalled); + func(); + t.ok(extension.status.goCalled); - // 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. - t.equal( - vm.runtime._blockInfo[0].menus[0].json.args0[0].options.length, 3); - // Second menu is a dynamic menu and therefore should be a function. - t.type( - vm.runtime._blockInfo[0].menus[1].json.args0[0].options, 'function'); - }); + // 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. + t.equal( + vm.runtime._blockInfo[0].menus[0].json.args0[0].options.length, 3); + // Second menu is a dynamic menu and therefore should be a function. + t.type( + vm.runtime._blockInfo[0].menus[1].json.args0[0].options, 'function'); + + t.end(); +}); + +test('load sync', t => { + const vm = new VirtualMachine(); + vm.extensionManager.loadExtensionIdSync('coreExample'); + t.ok(vm.extensionManager.isExtensionLoaded('coreExample')); + + t.equal(vm.runtime._blockInfo.length, 1); + t.equal(vm.runtime._blockInfo[0].blocks.length, 1); + t.type(vm.runtime._blockInfo[0].blocks[0].info, 'object'); + t.equal(vm.runtime._blockInfo[0].blocks[0].info.opcode, 'exampleOpcode'); + t.equal(vm.runtime._blockInfo[0].blocks[0].info.blockType, 'reporter'); + + // Test the opcode function + t.equal(vm.runtime._blockInfo[0].blocks[0].info.func(), 'no stage yet'); + + const sprite = new Sprite(null, vm.runtime); + sprite.name = 'Stage'; + const stage = new RenderedTarget(sprite, vm.runtime); + stage.isStage = true; + vm.runtime.targets = [stage]; + + t.equal(vm.runtime._blockInfo[0].blocks[0].info.func(), 'Stage'); + + t.end(); }); diff --git a/test/unit/dispatch.js b/test/unit/dispatch.js index 92ed58f0d..f3b944072 100644 --- a/test/unit/dispatch.js +++ b/test/unit/dispatch.js @@ -62,3 +62,24 @@ test('remote', t => { .then(() => runServiceTest('RemoteDispatchTest', t), e => t.fail(e)) .then(() => dispatch._remoteCall(worker, 'dispatch', 'terminate'), e => t.fail(e)); }); + +test('local, sync', t => { + dispatch.setServiceSync('SyncDispatchTest', new DispatchTestService()); + + const a = dispatch.callSync('SyncDispatchTest', 'returnFortyTwo'); + t.equal(a, 42); + + const b = dispatch.callSync('SyncDispatchTest', 'doubleArgument', 9); + t.equal(b, 18); + + const c = dispatch.callSync('SyncDispatchTest', 'doubleArgument', 123); + t.equal(c, 246); + + try { + dispatch.callSync('SyncDispatchTest', 'throwException'); + } catch (e) { + t.equal(e.message, 'This is a test exception thrown by DispatchTest'); + } + + t.end(); +}); From efcb801fe3a5b516e54c69095d0701cb4304261b Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Mon, 25 Mar 2019 16:32:51 -0400 Subject: [PATCH 4/7] Apply suggestions from code review Add error cases in new functions and remove todo comment. Co-Authored-By: kchadha --- src/dispatch/central-dispatch.js | 4 ++-- src/extension-support/extension-manager.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dispatch/central-dispatch.js b/src/dispatch/central-dispatch.js index 00c6ea77e..fe4987a6e 100644 --- a/src/dispatch/central-dispatch.js +++ b/src/dispatch/central-dispatch.js @@ -47,12 +47,12 @@ class CentralDispatch extends SharedDispatch { const {provider, isRemote} = this._getServiceProvider(service); if (provider) { if (isRemote) { - log.error("Cannot use 'callSync' on a remote service provider."); - return; + throw new Error(`Cannot use 'callSync' on remote provider for service ${service}.`); } return provider[method].apply(provider, args); } + throw new Error(`Provider not found for service: ${service}`); } /** diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index b5d1c55b0..3b8f25a69 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -123,7 +123,7 @@ class ExtensionManager { if (this.isExtensionLoaded(extensionId)) { const message = `Rejecting attempt to load a second extension with ID ${extensionId}`; log.warn(message); - return; // TODO Do we want to throw an error here? + return; } const extension = builtinExtensions[extensionId]; From 061b0b081f555cbe5c4990eda98568d9c80d9bd1 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Mon, 25 Mar 2019 16:39:00 -0400 Subject: [PATCH 5/7] Refactor loadExtensionURL for readability. --- src/extension-support/extension-manager.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index 3b8f25a69..5ffefa1b7 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -149,9 +149,9 @@ class ExtensionManager { const extension = builtinExtensions[extensionURL](); const extensionInstance = new extension(this.runtime); - return Promise.resolve(this._registerInternalExtension(extensionInstance)).then(serviceName => { - this._loadedExtensions.set(extensionURL, serviceName); - }); + const serviceName = this._registerInternalExtension(extensionInstance); + this._loadedExtensions.set(extensionURL, serviceName); + return Promise.resolve(); } return new Promise((resolve, reject) => { From eccdeff2ce969959cb733465652bdb2a82abf76c Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Tue, 26 Mar 2019 12:03:00 -0400 Subject: [PATCH 6/7] Use async require with coreExample extension. Log a warning when attempting to load a non-built in extension synchronously. Simplify unit test. --- src/extension-support/extension-manager.js | 29 ++++++++++++---------- test/unit/dispatch.js | 7 ++---- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index 5ffefa1b7..341f2461c 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -11,7 +11,7 @@ const BlockType = require('./block-type'); const builtinExtensions = { // This is an example that isn't loaded with the other core blocks, // but serves as a reference for loading core blocks as extensions. - coreExample: require('../blocks/scratch3_core_example'), + coreExample: () => require('../blocks/scratch3_core_example'), // These are the non-core built-in extensions. pen: () => require('../extensions/scratch3_pen'), wedo2: () => require('../extensions/scratch3_wedo2'), @@ -118,19 +118,22 @@ class ExtensionManager { * @param {string} extensionId - the ID of an internal extension */ loadExtensionIdSync (extensionId) { - if (builtinExtensions.hasOwnProperty(extensionId)) { - /** @TODO dupe handling for non-builtin extensions. See commit 670e51d33580e8a2e852b3b038bb3afc282f81b9 */ - if (this.isExtensionLoaded(extensionId)) { - const message = `Rejecting attempt to load a second extension with ID ${extensionId}`; - log.warn(message); - return; - } - - const extension = builtinExtensions[extensionId]; - const extensionInstance = new extension(this.runtime); - const serviceName = this._registerInternalExtension(extensionInstance); - this._loadedExtensions.set(extensionId, serviceName); + if (!builtinExtensions.hasOwnProperty(extensionId)) { + log.warn(`Could not find extension ${extensionId} in the built in extensions.`); + return; } + + /** @TODO dupe handling for non-builtin extensions. See commit 670e51d33580e8a2e852b3b038bb3afc282f81b9 */ + if (this.isExtensionLoaded(extensionId)) { + const message = `Rejecting attempt to load a second extension with ID ${extensionId}`; + log.warn(message); + return; + } + + const extension = builtinExtensions[extensionId](); + const extensionInstance = new extension(this.runtime); + const serviceName = this._registerInternalExtension(extensionInstance); + this._loadedExtensions.set(extensionId, serviceName); } /** diff --git a/test/unit/dispatch.js b/test/unit/dispatch.js index f3b944072..dfd00155e 100644 --- a/test/unit/dispatch.js +++ b/test/unit/dispatch.js @@ -75,11 +75,8 @@ test('local, sync', t => { const c = dispatch.callSync('SyncDispatchTest', 'doubleArgument', 123); t.equal(c, 246); - try { - dispatch.callSync('SyncDispatchTest', 'throwException'); - } catch (e) { - t.equal(e.message, 'This is a test exception thrown by DispatchTest'); - } + t.throws(() => dispatch.callSync('SyncDispatchTest', 'throwException'), + new Error('This is a test exception thrown by DispatchTest')); t.end(); }); From 2fbd152c5320a16bbac77fa52497f10f1dbb32d3 Mon Sep 17 00:00:00 2001 From: Karishma Chadha Date: Wed, 27 Mar 2019 17:42:10 -0400 Subject: [PATCH 7/7] Make loadExtensionURL consistent with error handling logic in loadExtensionIdSync --- src/extension-support/extension-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extension-support/extension-manager.js b/src/extension-support/extension-manager.js index 341f2461c..e28277efb 100644 --- a/src/extension-support/extension-manager.js +++ b/src/extension-support/extension-manager.js @@ -147,7 +147,7 @@ class ExtensionManager { if (this.isExtensionLoaded(extensionURL)) { const message = `Rejecting attempt to load a second extension with ID ${extensionURL}`; log.warn(message); - return Promise.reject(new Error(message)); + return Promise.resolve(); } const extension = builtinExtensions[extensionURL]();