Skip to content
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

Jump to bottom

refactor: convert some classes to ES6 classes #5928

Merged
merged 9 commits into from
Feb 16, 2022
Merged

refactor: convert some classes to ES6 classes #5928

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b01b042
refactor: move optional function declarations into the constructor
rachel-fenichel Feb 10, 2022
300c259
refactor: convert options.js to es6 class
rachel-fenichel Feb 10, 2022
51c3956
refactor: convert generator.js to es6 class
rachel-fenichel Feb 10, 2022
f741d21
refactor: convert workspace_events.js to es6 class
rachel-fenichel Feb 10, 2022
cfe0d3e
refactor: convert events_abstract.js to an es6 class
rachel-fenichel Feb 11, 2022
19b254b
chore: format
rachel-fenichel Feb 11, 2022
de7af56
chore: fix lint
rachel-fenichel Feb 11, 2022
46f7ffd
chore: rebuild
rachel-fenichel Feb 11, 2022
f7950c0
chore: respond to PR feedback
rachel-fenichel Feb 16, 2022
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
3 changes: 2 additions & 1 deletion core/events/events_abstract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ class Abstract {

/**
* Run an event.
* @param {boolean} _forward True if run forward, false if run backward (undo).
* @param {boolean} _forward True if run forward, false if run backward
* (undo).
*/
run(_forward) {
// Defined by subclasses.
Expand Down
31 changes: 17 additions & 14 deletions core/generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ class Generator {
workspaceToCode(workspace) {
if (!workspace) {
// Backwards compatibility from before there could be multiple workspaces.
console.warn('No workspace specified in workspaceToCode call. Guessing.');
console.warn(
'No workspace specified in workspaceToCode call. Guessing.');
workspace = common.getMainWorkspace();
}
let code = [];
Expand Down Expand Up @@ -220,7 +221,8 @@ class Generator {
* Generate code for the specified block (and attached blocks).
* The generator must be initialized before calling this function.
* @param {Block} block The block to generate code for.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't change this, but while you're here this needs a ?. I guess this doesn't matter as long as the Typescript conversion tool that we eventually use still knows that this is nullable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

* @param {boolean=} opt_thisOnly True to generate code for only this statement.
* @param {boolean=} opt_thisOnly True to generate code for only this
* statement.
* @return {string|!Array} For statement blocks, the generated code.
* For value blocks, an array containing the generated code and an
* operator order value. Returns '' if block is null.
Expand Down Expand Up @@ -251,7 +253,8 @@ class Generator {
// First argument to func.call is the value of 'this' in the generator.
// Prior to 24 September 2013 'this' was the only way to access the block.
// The current preferred method of accessing the block is through the second
// argument to func.call, which becomes the first parameter to the generator.
// argument to func.call, which becomes the first parameter to the
// generator.
let code = func.call(block, block);
if (Array.isArray(code)) {
// Value blocks return tuples of code and operator order.
Expand All @@ -278,8 +281,8 @@ class Generator {
* Generate code representing the specified value input.
* @param {!Block} block The block containing the input.
* @param {string} name The name of the input.
* @param {number} outerOrder The maximum binding strength (minimum order value)
* of any operators adjacent to "block".
* @param {number} outerOrder The maximum binding strength (minimum order
* value) of any operators adjacent to "block".
* @return {string} Generated code or '' if no blocks are connected or the
* specified input does not exist.
*/
Expand Down Expand Up @@ -421,10 +424,10 @@ class Generator {

/**
* Define a developer-defined function (not a user-defined procedure) to be
* included in the generated code. Used for creating private helper functions.
* The first time this is called with a given desiredName, the code is
* saved and an actual name is generated. Subsequent calls with the
* same desiredName have no effect but have the same return value.
* included in the generated code. Used for creating private helper
* functions. The first time this is called with a given desiredName, the code
* is saved and an actual name is generated. Subsequent calls with the same
* desiredName have no effect but have the same return value.
*
* It is up to the caller to make sure the same desiredName is not
* used for different helper functions (e.g. use "colourRandom" and
Expand Down Expand Up @@ -485,9 +488,9 @@ class Generator {
/**
* Common tasks for generating code from blocks. This is called from
* blockToCode and is called on every block, not just top level blocks.
* Subclasses may override this, e.g. to generate code for statements following
* the block, or to handle comments for the specified block and any connected
* value blocks.
* Subclasses may override this, e.g. to generate code for statements
* following the block, or to handle comments for the specified block and any
* connected value blocks.
* @param {!Block} _block The current block.
* @param {string} code The code created for this block.
* @param {boolean=} _opt_thisOnly True to generate code for only this
Expand All @@ -502,8 +505,8 @@ class Generator {

/**
* Hook for code to run at end of code generation.
* Subclasses may override this, e.g. to prepend the generated code with import
* statements or variable definitions.
* Subclasses may override this, e.g. to prepend the generated code with
* import statements or variable definitions.
* @param {string} code Generated code.
* @return {string} Completed code.
*/
Expand Down
3 changes: 2 additions & 1 deletion core/mutator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,8 @@ class Mutator extends Icon {
*/
workspaceChanged_(e) {
if (!(e.isUiEvent ||
(e.type === eventUtils.CHANGE && /** @type {!BlockChange} */(e).element === 'disabled') ||
(e.type === eventUtils.CHANGE &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct me if I'm wrong, but I believe that we use the the type property this way because this code is mostly unchanged from the time before we had separate classes for each event. If we used e instanceof BlockChange instead of checking the type property, then we wouldn't have to cast anything because we take advantage of Closure's type refinement / TS's narrowing. It's even better for UI events since we can check if it's an instance of the UI base class and we don't need the isUiEvent function anymore. This goes for most of the casting you've had to do in this PR. I am curious if there is a reason that wouldn't work or if this is just a relic from the old way of doing events.

I assume that developers may be using the type property since events are frequently used externally. But if the event class and the type property are expected to be in sync then using the actual class name instead of the property might be easier to manage for most internal use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other reason to use type instead of instanceof: I have to requireType the appropriate event for the cast, but don't have to require it. That's true here and in any other place I might use instanceof.

Do you see any downsides to changing to require? It makes that type of event a required dependency that can't be shaken out, but I think that's fine for events.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried changing to require and it broke compilation because there is a circular dependency that it can't break. I'm going to punt this and continue using type instead of instanceof.

/** @type {!BlockChange} */ (e).element === 'disabled') ||
e.type === eventUtils.CREATE)) {
this.updateWorkspace_();
}
Expand Down
18 changes: 10 additions & 8 deletions core/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,10 @@ class Options {

/**
* If set, sets the translation of the workspace to match the scrollbars.
* @type {undefined|function(!{x:number,y:number}):void} A function that sets
* the translation of the workspace to match the scrollbars. The argument
* Contains an x and/or y property which is a float between 0 and 1
* specifying the degree of scrolling.
* @type {undefined|function(!{x:number,y:number}):void} A function that
* sets the translation of the workspace to match the scrollbars. The
* argument Contains an x and/or y property which is a float between 0
* and 1 specifying the degree of scrolling.
*/
this.setMetrics = undefined;

Expand All @@ -217,7 +217,8 @@ class Options {
static parseMoveOptions_(options, hasCategories) {
const move = options['move'] || {};
const moveOptions = {};
if (move['scrollbars'] === undefined && options['scrollbars'] === undefined) {
if (move['scrollbars'] === undefined &&
options['scrollbars'] === undefined) {
moveOptions.scrollbars = hasCategories;
} else if (typeof move['scrollbars'] === 'object') {
moveOptions.scrollbars = {};
Expand All @@ -226,7 +227,8 @@ class Options {
// Convert scrollbars object to boolean if they have the same value.
// This allows us to easily check for whether any scrollbars exist using
// !!moveOptions.scrollbars.
if (moveOptions.scrollbars.horizontal && moveOptions.scrollbars.vertical) {
if (moveOptions.scrollbars.horizontal &&
moveOptions.scrollbars.vertical) {
moveOptions.scrollbars = true;
} else if (
!moveOptions.scrollbars.horizontal &&
Expand Down Expand Up @@ -323,8 +325,8 @@ class Options {
}

/**
* Parse the user-specified theme options, using the classic theme as a default.
* https://developers.google.com/blockly/guides/configure/web/themes
* Parse the user-specified theme options, using the classic theme as a
* default. https://developers.google.com/blockly/guides/configure/web/themes
* @param {!Object} options Dictionary of options.
* @return {!Theme} A Blockly Theme.
* @private
Expand Down
3 changes: 2 additions & 1 deletion core/trashcan.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,8 @@ class Trashcan extends DeleteArea {
* @private
*/
onDelete_(event) {
if (this.workspace_.options.maxTrashcanContents <= 0 || event.type !== eventUtils.BLOCK_DELETE) {
if (this.workspace_.options.maxTrashcanContents <= 0 ||
event.type !== eventUtils.BLOCK_DELETE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd even consider using instanceof here too just for consistency, though it's less beneficial when we don't need the type-narrowing.

return;
}
const deleteEvent = /** @type {!BlockDelete} */ (event);
Expand Down
3 changes: 2 additions & 1 deletion core/workspace_comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ class WorkspaceComment {
* @package
*/
moveBy(dx, dy) {
const event = /** @type {!CommentMove} */(new (eventUtils.get(eventUtils.COMMENT_MOVE))(this));
const event = /** @type {!CommentMove} */ (
new (eventUtils.get(eventUtils.COMMENT_MOVE))(this));
this.xy_.translate(dx, dy);
event.recordNew();
eventUtils.fire(event);
Expand Down
3 changes: 2 additions & 1 deletion core/workspace_comment_svg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,8 @@ class WorkspaceCommentSvg extends WorkspaceComment {
* @package
*/
moveBy(dx, dy) {
const event = /** @type {!CommentMove} */(new (eventUtils.get(eventUtils.COMMENT_MOVE))(this));
const event = /** @type {!CommentMove} */ (
new (eventUtils.get(eventUtils.COMMENT_MOVE))(this));
// TODO: Do I need to look up the relative to surface XY position here?
const xy = this.getRelativeToSurfaceXY();
this.translate(xy.x + dx, xy.y + dy);
Expand Down