Move main workspace to a new file, common.js

[rachel-fenichel]

The basics

• I branched from goog_module
• My pull request is against goog_module
• My code follows the style guide

The details

Resolves

Part of #5208

Proposed Changes

Moves a function and property from blockly.js to a new leaf namespace named Blockly.common, and adds aliases to keep the old function and property accessible.

@samelhusseini pointed out that as long as I get rid of the call to goog.module.declareLegacyNamespace() before the next release, Blockly.common will never be externally visible, so an overly broad name like common does not add to our API worries. I therefore propose to move the other parts of blockly.js into this file to help resolve circular dependencies.

I did add a call to declareLegacyNamespace so that I don't have to simultaneously convert all other files that use these properties and functions. I will need to remove that at the end of the quarter.

Blockly.mainWorkspace

I moved the Blockly.mainWorkspace to common.js and did not export it. Instead I exported two functions, getMainWorkspace and setMainWorkspace. I also aliased Blockly.getMainWorkspace to refer to Blockly.common.getMainWorkspace.

Getter and setter

In blockly.js I used Object.defineProperty to add a getter and setter for mainWorkspace to the Blockly object:

// Add a getter and setter pair for Blockly.mainWorkspace, for legacy reasons.
Object.defineProperty(Blockly, 'mainWorkspace', {
  set: function(x) {Blockly.common.setMainWorkspace(x);},
  get: function() {return Blockly.common.getMainWorkspace();}
});

Open question: how do I add jsdoc to this?

I confirmed that the getter and setter work by using the playground and setting the property in the console, then verifying that Blockly.common.mainWorkspace updated appropriately.

I think this pattern works for dealing with properties that could beset externally. I also think that this property should not be externally visible, and I propose that I add deprecation warnings for both the getter and the setter.

However, Blockly.getMainWorkspace() should remain public, since we've used it extensively in demos.

Initialization change

In blockly.js this property was originally set to null, and then set to a real workspace as soon as a workspace was injected. Later uses of mainWorkspace call getMainWorkspace, which casts it to a non-nullable Workspace in order to avoid propagating this initial null through the rest of the codebase.

In this case, I instead declared mainWorkspace as a non-nullable Workspace, and set it through a function that accepts a non-nullable Workspace as an argument. This removed the need for the cast on the getter. I think this is safe because trying to access the main workspace before it is first set will throw an error, but I want an extra eye on it.

Update uses

The second commit updates uses of Blockly.mainWorkspace and Blockly.getMainWorkspace() in core to use the versions in common.js.

[BeksOmega]

Instead I exported two functions, getMainWorkspace and setMainWorkspace.

Do we want to provide this as part of our API? Developers can already grab a reference to the workspace when they inject it. I think this only makes things confusing when you start dealing with multiple workspaces. I know you said we use this is demos, but I feel like it would be better to update those internal usages than to keep this around.

Not a strong opinion, just my feeling on it.

[rachel-fenichel]

I feel like it would be better to update those internal usages than to keep this around.

I agree that we should update all of our internal usages, which includes the demos. I mentioned the demos because those are representative of suggestions we've made to developers in the past.

Note that Blockly.common.setMainWorkspace and Blockly.common.setMainWorkspace will need to be exported from this module for internal use in any case, so the question we're discussing here is whether we should deprecate the existing Blockly.getMainWorkspace function.

I'd be down to also mark Blockly.getMainWorkspace as deprecated with a warning that we'll remove it in a year.