Migrate core/events/events.js to goog.module syntax #5302
Conversation
github-actions
bot
added
type: cleanup
and removed
cla: yes
|
|
| Blockly.Events.COMMENT_MOVE | ||
| ]; | ||
| const BUMP_EVENTS = [BLOCK_CREATE, BLOCK_MOVE, COMMENT_CREATE, COMMENT_MOVE]; | ||
| exports.BUMP_EVENTS = BUMP_EVENTS; |
There was a problem hiding this comment.
Would it be evil to just say:
exports.BUMP_EVENTS = [exports.BLOCK_CREATE, exports.BLOCK_MOVE, exports.COMMENT_CREATE, exports.COMMENT_MOVE]
Then 27 different const statements could disappear.
There was a problem hiding this comment.
I don't think I quite understand the suggestion here, could you clarify?
There was a problem hiding this comment.
I think @NeilFraser is suggesting that it is probably worth deviating from our usual convention of declare-then-export for all of these trivial declarations—I made a very similar suggestion about the flags exported by useragent.js in #5396.
Though in this case I smell a missing enum…
| Blockly.Events.filter = function(queueIn, forward) { | ||
| var queue = queueIn.slice(); // Shallow copy of queue. | ||
| const filter = function(queueIn, forward) { | ||
| let queue = queueIn.slice(); // Shallow copy of queue. |
There was a problem hiding this comment.
I think queue is const. The reverse is an in place operation.
There was a problem hiding this comment.
It gets reassigned on line 409.
| Blockly.Events.disabled_ = 0; | ||
| let disabled = 0; | ||
| /** @private */ | ||
| exports.disabled_ = disabled; |
There was a problem hiding this comment.
This line appears to be clobbered by the setter below. Delete?
There was a problem hiding this comment.
Closure isn't able to see properties that get added by Object.defineProperties, so this is here solely to inform the compiler that a .disabled_ property exists.
|
|
||
| Object.defineProperties(exports, { | ||
| disabled_: { | ||
| set: function(newValue) { |
There was a problem hiding this comment.
Who calls this setter? Seems odd to export a private property. I don't see anyone using this in our codebase in develop.
There was a problem hiding this comment.
It's used by test_helpers.js and workspace_test.js unfortunately.
| */ | ||
| Blockly.Events.BumpEvent; | ||
| let BumpEvent; | ||
| exports.BumpEvent = BumpEvent; |
There was a problem hiding this comment.
This is a typedef, does this need to be a let variable?
There was a problem hiding this comment.
The compiler complains if it's made a const because it isn't being assigned a value.
3 participants
The basics
goog_modulegoog_moduleconversion guide
npm test.The details
Resolves
Part of #5026
Proposed Changes
Converts
core/events/events.jstogoog.modulewith ES6const/let.Additional Information
This uses goog.forwardDeclare/goog.module.get to work around circular requires with Blockly.Workspace.