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

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 8 commits
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
160 changes: 85 additions & 75 deletions core/events/events_abstract.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,98 +24,108 @@ const {Workspace} = goog.requireType('Blockly.Workspace');

/**
* Abstract class for an event.
* @constructor
* @alias Blockly.Events.Abstract
*/
const Abstract = function() {
class Abstract {
Comment on lines 28 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be declared @abstract, either in this PR or a subsequent one?

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.

/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {?boolean}
* @alias Blockly.Events.Abstract
*/
this.isBlank = null;
constructor() {
/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {?boolean}
*/
this.isBlank = null;

/**
* The workspace identifier for this event.
* @type {string|undefined}
*/
this.workspaceId = undefined;

/**
* The event group id for the group this event belongs to. Groups define
* events that should be treated as an single action from the user's
* perspective, and should be undone together.
* @type {string}
*/
this.group = eventUtils.getGroup();

/**
* Sets whether the event should be added to the undo stack.
* @type {boolean}
*/
this.recordUndo = eventUtils.getRecordUndo();

/**
* Whether or not the event is a UI event.
* @type {boolean}
*/
this.isUiEvent = false;

/**
* Type of this event.
* @type {string|undefined}
*/
this.type = undefined;
Comment on lines +66 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be preferable if .type had @type {string}, rather than allowing undefined. What about adding a type parameter to the constructor and thereby force subclasses to provide a type tag when they call super?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to note, I'm pretty sure we let external peeps define events (I think AppInventor does this?)

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 think we could reasonably go to a string and have the empty string if not set--but when I tried that assorted tests broke, mostly because of assertions about what the type property was, so I left it alone in the interest of making a minimal change. I expect to return to this to address typescript compiler warnings about this value possibly being undefined.

}

/**
* The workspace identifier for this event.
* @type {string|undefined}
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
this.workspaceId = undefined;
toJson() {
const json = {'type': this.type};
if (this.group) {
json['group'] = this.group;
}
return json;
}

/**
* The event group id for the group this event belongs to. Groups define
* events that should be treated as an single action from the user's
* perspective, and should be undone together.
* @type {string}
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
this.group = eventUtils.getGroup();
fromJson(json) {
this.isBlank = false;
this.group = json['group'];
}

/**
* Sets whether the event should be added to the undo stack.
* @type {boolean}
* Does this event record any change of state?
* @return {boolean} True if null, false if something changed.
*/
this.recordUndo = eventUtils.getRecordUndo();
isNull() {
return false;
}

/**
* Whether or not the event is a UI event.
* @type {boolean}
* Run an event.
* @param {boolean} _forward True if run forward, false if run backward
* (undo).
*/
this.isUiEvent = false;
};

/**
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
Abstract.prototype.toJson = function() {
const json = {'type': this.type};
if (this.group) {
json['group'] = this.group;
run(_forward) {
// Defined by subclasses.
}
return json;
};

/**
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
Abstract.prototype.fromJson = function(json) {
this.isBlank = false;
this.group = json['group'];
};

/**
* Does this event record any change of state?
* @return {boolean} True if null, false if something changed.
*/
Abstract.prototype.isNull = function() {
return false;
};

/**
* Run an event.
* @param {boolean} _forward True if run forward, false if run backward (undo).
*/
Abstract.prototype.run = function(_forward) {
// Defined by subclasses.
};

/**
* Get workspace the event belongs to.
* @return {!Workspace} The workspace the event belongs to.
* @throws {Error} if workspace is null.
* @protected
*/
Abstract.prototype.getEventWorkspace_ = function() {
let workspace;
if (this.workspaceId) {
const {Workspace} = goog.module.get('Blockly.Workspace');
workspace = Workspace.getById(this.workspaceId);
}
if (!workspace) {
throw Error(
'Workspace is null. Event must have been generated from real' +
' Blockly events.');
/**
* Get workspace the event belongs to.
* @return {!Workspace} The workspace the event belongs to.
* @throws {Error} if workspace is null.
* @protected
*/
getEventWorkspace_() {
let workspace;
if (this.workspaceId) {
const {Workspace} = goog.module.get('Blockly.Workspace');
workspace = Workspace.getById(this.workspaceId);
}
if (!workspace) {
throw Error(
'Workspace is null. Event must have been generated from real' +
' Blockly events.');
}
return workspace;
}
return workspace;
};
}

exports.Abstract = Abstract;
35 changes: 23 additions & 12 deletions core/events/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const registry = goog.require('Blockly.registry');
/* eslint-disable-next-line no-unused-vars */
const {Abstract} = goog.requireType('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
const {BlockChange} = goog.requireType('Blockly.Events.BlockChange');
/* eslint-disable-next-line no-unused-vars */
const {BlockCreate} = goog.requireType('Blockly.Events.BlockCreate');
/* eslint-disable-next-line no-unused-vars */
const {BlockMove} = goog.requireType('Blockly.Events.BlockMove');
Expand All @@ -32,6 +34,8 @@ const {CommentCreate} = goog.requireType('Blockly.Events.CommentCreate');
/* eslint-disable-next-line no-unused-vars */
const {CommentMove} = goog.requireType('Blockly.Events.CommentMove');
/* eslint-disable-next-line no-unused-vars */
const {ViewportChange} = goog.requireType('Blockly.Events.ViewportChange');
/* eslint-disable-next-line no-unused-vars */
const {Workspace} = goog.requireType('Blockly.Workspace');


Expand Down Expand Up @@ -307,6 +311,7 @@ exports.BUMP_EVENTS = BUMP_EVENTS;

/**
* List of events queued for firing.
* @type {!Array<!Abstract>}
*/
const FIRE_QUEUE = [];

Expand Down Expand Up @@ -365,7 +370,9 @@ const filter = function(queueIn, forward) {
if (!event.isNull()) {
// Treat all UI events as the same type in hash table.
const eventType = event.isUiEvent ? UI : event.type;
const key = [eventType, event.blockId, event.workspaceId].join(' ');
// TODO(#5927): Ceck whether `blockId` exists before accessing it.
const blockId = /** @type {BlockMove} */ (event).blockId;
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels weird... Even if this is temporary I really dislike just casting any random event to a BlockMove type.

I'm assuming this is done to silence a TypeScript error about AbstractEvent not having a property blockId. If so, then is that error a blocking problem? If not then I think it should throw an error (because it is) until we can actually fix this. If it is blocking progress then would casting it to * or ? work? I would guess that a TS conversion would treat that as casting to any which can then be caught more easily by automated tooling such as eslint.

If that's not the reason, could you explain a bit more what's happening here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually for a Closure Compiler error, rather than a Typescript error. It's triggered by changing the base class to an ES6 class, which caused stricter type checks all around.

I filed #5927 to look into this because I couldn't get it to work by just adding blockId to the base class--there's an inconsistency in how blockId is annotated across different subclasses that pops up when I add it to the base class.

What I actually want to say if: in this hash, use undefined for the blockId if it doesn't exist on this class, and otherwise use whatever value is found. But I can't check whether this object has that property in a way that Closure compiler agrees with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Best to be explicit:

Suggested change
const blockId = /** @type {BlockMove} */ (event).blockId;
const blockId = (event instanceof BlockMove) ? event.blockId : undefined;

Or if there are multiple subtypes with a .blockId property:

Suggested change
const blockId = /** @type {BlockMove} */ (event).blockId;
const blockId = (event instanceof BlockMove) ? event.blockId :
(event instanceof EventType1) ? event.blockId :
(event instanceof EventType2) ? event.blockId : undefined;

…though this kind of smells, and it might be better just to fix the declaration of blockId so it can be moved to Abstract, or failing that provide a getBlockId method on Abstract with a well-specified return type that can be implemented by subtypes as required.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that Abstract should necessarily have any knowledge of block IDs, because not all events are associated with blocks. Eg zoom click events. Could blockId be added to the BlockBase event instead, and then that type can be used in the ternary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Beka's solution as the long-term solution that should be investigated as part of #5927. I think for the short-term bandaid solution I'm saying I would prefer to do

const blockId = /** @type {*} */ (event).blockId;

which I think should accomplish the same thing (silence the closure error) but be easier to find as a problem in the future if 5927 isn't picked up relatively soon. I say easier to find because I assume this would get translated in TS to a cast as any and I know for a fact that the eslint config we use has a setting to make that an error (we have it as such in samples) and I assume there might be an equivalent rule for closure.

Since we don't mind at all if blockId is undefined, we don't actually need the type checking here. I think listing out the individual types is actually more fragile in this case because if we forget one type then those events will not work properly. Therefore I don't think we should go with that suggestion even in the short-term band-aid solution.

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'll swap it to {*} for this PR. And yes, down the line the typescript compiler will force me to sort this out.

const key = [eventType, blockId, event.workspaceId].join(' ');

const lastEntry = hash[key];
const lastEvent = lastEntry ? lastEntry.event : null;
Expand All @@ -376,22 +383,25 @@ const filter = function(queueIn, forward) {
hash[key] = {event: event, index: i};
mergedQueue.push(event);
} else if (event.type === MOVE && lastEntry.index === i - 1) {
const moveEvent = /** @type {!BlockMove} */ (event);
Comment on lines 385 to +386
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (event.type === MOVE && lastEntry.index === i - 1) {
const moveEvent = /** @type {!BlockMove} */ (event);
} else if (event instanceof BlockMove) && lastEntry.index === i - 1) {

and then you don't need the cast or moveEvent.

// Merge move events.
lastEvent.newParentId = event.newParentId;
lastEvent.newInputName = event.newInputName;
lastEvent.newCoordinate = event.newCoordinate;
lastEvent.newParentId = moveEvent.newParentId;
lastEvent.newInputName = moveEvent.newInputName;
lastEvent.newCoordinate = moveEvent.newCoordinate;
lastEntry.index = i;
} else if (
event.type === CHANGE && event.element === lastEvent.element &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
event.type === CHANGE && event.element === lastEvent.element &&
(event.type instanceof BlockChange) && event.element === lastEvent.element &&

and dispense with the cast and changeEvent.

event.name === lastEvent.name) {
const changeEvent = /** @type {!BlockChange} */ (event);
// Merge change events.
lastEvent.newValue = event.newValue;
lastEvent.newValue = changeEvent.newValue;
} else if (event.type === VIEWPORT_CHANGE) {
const viewportEvent = /** @type {!ViewportChange} */ (event);
// Merge viewport change events.
lastEvent.viewTop = event.viewTop;
lastEvent.viewLeft = event.viewLeft;
lastEvent.scale = event.scale;
lastEvent.oldScale = event.oldScale;
lastEvent.viewTop = viewportEvent.viewTop;
lastEvent.viewLeft = viewportEvent.viewLeft;
lastEvent.scale = viewportEvent.scale;
lastEvent.oldScale = viewportEvent.oldScale;
} else if (event.type === CLICK && lastEvent.type === BUBBLE_OPEN) {
// Drop click events caused by opening/closing bubbles.
} else {
Expand Down Expand Up @@ -546,12 +556,13 @@ exports.get = get;
*/
const disableOrphans = function(event) {
if (event.type === MOVE || event.type === CREATE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (event.type === MOVE || event.type === CREATE) {
if ((event instanceof BlockMove) || (event instanceof BlockCreate)) {

and dispense with the cast and blockEvent.

if (!event.workspaceId) {
const blockEvent = /** @type {!BlockMove|!BlockCreate} */ (event);
if (!blockEvent.workspaceId) {
return;
}
const {Workspace} = goog.module.get('Blockly.Workspace');
const eventWorkspace = Workspace.getById(event.workspaceId);
let block = eventWorkspace.getBlockById(event.blockId);
const eventWorkspace = Workspace.getById(blockEvent.workspaceId);
let block = eventWorkspace.getBlockById(blockEvent.blockId);
if (block) {
// Changing blocks as part of this event shouldn't be undoable.
const initialUndoFlag = recordUndo;
Expand Down
102 changes: 48 additions & 54 deletions core/events/workspace_events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
goog.module('Blockly.Events.FinishedLoading');

const eventUtils = goog.require('Blockly.Events.utils');
const object = goog.require('Blockly.utils.object');
const registry = goog.require('Blockly.registry');
const {Abstract} = goog.require('Blockly.Events.Abstract');
/* eslint-disable-next-line no-unused-vars */
Expand All @@ -28,70 +27,65 @@ const {Workspace} = goog.requireType('Blockly.Workspace');
* Used to notify the developer when the workspace has finished loading (i.e
* domToWorkspace).
* Finished loading events do not record undo or redo.
* @param {!Workspace=} opt_workspace The workspace that has finished
* loading. Undefined for a blank event.
* @extends {Abstract}
* @constructor
* @alias Blockly.Events.FinishedLoading
*/
const FinishedLoading = function(opt_workspace) {
class FinishedLoading extends Abstract {
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: it might be helpful for readability if the superclass were imported as AbstractEvent.

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

/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {boolean}
* @param {!Workspace=} opt_workspace The workspace that has finished
* loading. Undefined for a blank event.
* @alias Blockly.Events.FinishedLoading
*/
this.isBlank = typeof opt_workspace === 'undefined';
constructor(opt_workspace) {
super();
/**
* Whether or not the event is blank (to be populated by fromJson).
* @type {boolean}
*/
this.isBlank = typeof opt_workspace === 'undefined';

/**
* The workspace identifier for this event.
* @type {string}
*/
this.workspaceId = opt_workspace ? opt_workspace.id : '';

// Workspace events do not undo or redo.
this.recordUndo = false;

/**
* Type of this event.
* @type {string}
*/
this.type = eventUtils.FINISHED_LOADING;
}

/**
* The workspace identifier for this event.
* @type {string}
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
this.workspaceId = opt_workspace ? opt_workspace.id : '';
toJson() {
const json = {
'type': this.type,
};
if (this.group) {
json['group'] = this.group;
}
if (this.workspaceId) {
json['workspaceId'] = this.workspaceId;
}
return json;
}

/**
* The event group ID for the group this event belongs to. Groups define
* events that should be treated as an single action from the user's
* perspective, and should be undone together.
* @type {string}
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
this.group = eventUtils.getGroup();

// Workspace events do not undo or redo.
this.recordUndo = false;
};
object.inherits(FinishedLoading, Abstract);

/**
* Type of this event.
* @type {string}
*/
FinishedLoading.prototype.type = eventUtils.FINISHED_LOADING;

/**
* Encode the event as JSON.
* @return {!Object} JSON representation.
*/
FinishedLoading.prototype.toJson = function() {
const json = {
'type': this.type,
};
if (this.group) {
json['group'] = this.group;
fromJson(json) {
this.isBlank = false;
this.workspaceId = json['workspaceId'];
this.group = json['group'];
}
if (this.workspaceId) {
json['workspaceId'] = this.workspaceId;
}
return json;
};

/**
* Decode the JSON event.
* @param {!Object} json JSON representation.
*/
FinishedLoading.prototype.fromJson = function(json) {
this.isBlank = false;
this.workspaceId = json['workspaceId'];
this.group = json['group'];
};
}

registry.register(
registry.Type.EVENT, eventUtils.FINISHED_LOADING, FinishedLoading);
Expand Down
Loading