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.

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

Migrate core/utils/coordinate.js to goog.module syntax #5047

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

moniika
Copy link
Contributor

@moniika moniika commented Jul 14, 2021

The basics

  • I branched from goog_module
  • My pull request is against goog_module
  • My code follows the style guide
  • My code is presented in the form suggested in the module
    conversion guide
  • I have run npm test locally already.

The details

Resolves

Part of #5026

Proposed Changes

Converts core/utils/coordinate.js to goog.module syntax.

Test Coverage

Ran npm run start and npm run test

Additional Information

No changes after running clang-format on this file.

@moniika moniika requested a review from a team as a code owner July 14, 2021 01:40
@moniika moniika requested a review from maribethb July 14, 2021 01:40
@moniika moniika added this to the 2021_q3_release milestone Jul 14, 2021
@moniika moniika changed the title Convert coordinate to goog.module syntax Migrate core/utils/coordinate.js to goog.module syntax Jul 14, 2021
@@ -46,7 +46,7 @@ Blockly.utils.Coordinate = function(x, y) {
* @param {?Blockly.utils.Coordinate} b A Coordinate.
* @return {boolean} True iff the coordinates are equal, or if both are null.
*/
Blockly.utils.Coordinate.equals = function(a, b) {
Coordinate.equals = function(a, b) {
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 these (all the ones that aren't on the prototype) should be
const equals = function(a, b) {...}

based on our discussion today, right? I think so, but this is the only one of the utils classes that mixes both prototype and non-prototype methods so I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

equals appears to be a static method on the Coordinate class, this appears to be correct to me.

this.x += tx;
this.y += ty;
return this;
};

exports = Coordinate;
Copy link
Contributor

Choose a reason for hiding this comment

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

and the functions not on the prototype (if above comment applies)

Copy link
Contributor

Choose a reason for hiding this comment

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

This module appears to be a single class, so exporting it this way is correct, at least in this first pass. Changing this to exports = {Coordinate, equals} would break code which currently assumes that doing goog.require('Blockly.utils.Coordinate'); gets you a Blockly.utils.Coordinate constructor function.

We could go the route of exporting the class and the function separately (and I would recommend that, were equals not so obviously suited to be a static method on Coordinate), but if we did so the module name should be changed to Blockly.utils.coordinate and code which wanted to use the constructor would have to be changed import const {Coordinate} = goog.require('Blockly.utils.coordinate');

I'll update the conversion guide to clarify, and also file a bug to track cases where we might want to do such renaming / unbundling.

core/utils/coordinate.js Outdated Show resolved Hide resolved
@moniika
Copy link
Contributor Author

moniika commented Jul 16, 2021

I've rebased so that I could fix the merge conflicts but still keep the commits organized.

@moniika moniika requested a review from maribethb July 16, 2021 00:59
@moniika moniika merged commit 972f302 into google:goog_module Jul 16, 2021
@moniika moniika deleted the convert-coordinate branch July 16, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants