Make constants.js have no side effects #5140

rachel-fenichel · 2021-07-20T21:59:48Z

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.

The details

Resolves

Part of #5026

Proposed Changes

Converts core/constants.js to a module with es6 const/let.
Removes all side-effect assignments from constants.js (any assignments to properties on the Blockly object).
Moves previously side-effect assigned properties onto a new namespace, Blockly.internalConstants.
Adds aliases in blockly.js for all of these properties.
Updates all references in the core library to require Blockly.internalConstants and use properties exported there.
Does not update anything in blocks/ or generators/, as those are outside of core.
Removes all lines of goog.require('Blockly.constants') that were tagged with @suppress extraRequire

Additional Information

Follow-up work includes:

Investigate each of the properties (now in the list of aliases at the bottom of blockly.js) to decide which ones are not actually public, if any.
Add deprecation warnings to any properties that are public but that we will remove in an upcoming release.
Add setters for any properties that we will keep visible past this quarter.

cpcallen

LGTM, except specific points below and:

Some of these files now definitely have invalid goog.requires ordering. I've noted a couple of these below, but there are several more. Do you want to fix them in this PR? An open issue to fix this problem globally would be an acceptable alternative.
Have you run clang-format on the affected files (at least core/constants.js and core/internal_constants.js)?

I've filed #5158 to track breaking up and removing core/internal_constants.js in due course.

Aside: I found this PR a bit difficult to review. Part of this was just that it is a bit larger than the usual migrations, but it didn't help that some of the intermediate commits left it in an obviously-broken state. I think I'd have preferred if it had been presented with the first commit creating internal_constants.js and doing all of the associated changes to other files (including updating deps.js) without changing line breaks / indentation, followed by the usual sequence of commits for updating constants.js to goog.module and named requires, then a final clang-format of everything.

cpcallen · 2021-07-21T19:32:06Z

core/internal_constants.js

+/**
+ * @fileoverview Module that provides constants for use inside Blockly. Do not
+ * use these constants outside of the core library.
+ * @author fenichel@google.com (Rachel Fenichel)
+ */


I think the whole file should be declared @package—see e.g..

cpcallen · 2021-07-21T19:51:52Z

core/internal_constants.js

+/**
+ * Lookup table for determining the opposite type of a connection.
+ * @const
+ */
+const OPPOSITE_TYPE = [];
+OPPOSITE_TYPE[connectionTypes.INPUT_VALUE] = connectionTypes.OUTPUT_VALUE;
+OPPOSITE_TYPE[connectionTypes.OUTPUT_VALUE] = connectionTypes.INPUT_VALUE;
+OPPOSITE_TYPE[connectionTypes.NEXT_STATEMENT] =
+    connectionTypes.PREVIOUS_STATEMENT;
+OPPOSITE_TYPE[connectionTypes.PREVIOUS_STATEMENT] =
+    connectionTypes.NEXT_STATEMENT;
+
+exports.OPPOSITE_TYPE = OPPOSITE_TYPE;


OK, this clearly belongs in connection_type.js and it should probably have a better name. This is what motivated me to file #5158.

cpcallen · 2021-07-21T19:54:02Z

core/connection_checker.js

  if (blockA == blockB) {
    return Connection.REASON_SELF_CONNECTION;
-  } else if (b.type != Blockly.OPPOSITE_TYPE[a.type]) {
+  } else if (b.type != OPPOSITE_TYPE[a.type]) {


This looks weird to me, but I've made a suggestion to fix it in #5158 so I don't think it merits further change in this PR.

cpcallen · 2021-07-21T20:03:23Z

core/generator.js

 goog.require('Blockly.Block');
-/** @suppress {extraRequire} */
-goog.require('Blockly.constants');
+goog.require('Blockly.internalConstants');


Require ordering.

cpcallen · 2021-07-21T20:03:31Z

core/gesture.js

 goog.require('Blockly.Events');
 /** @suppress {extraRequire} */
 goog.require('Blockly.Events.Click');
+goog.require('Blockly.internalConstants');


Require ordering.

rachel-fenichel · 2021-07-21T20:52:26Z

Aside: I found this PR a bit difficult to review. Part of this was just that it is a bit larger than the usual migrations, but it didn't help that some of the intermediate commits left it in an obviously-broken state. I think I'd have preferred if it had been presented with the first commit creating internal_constants.js and doing all of the associated changes to other files (including updating deps.js) without changing line breaks / indentation, followed by the usual sequence of commits for updating constants.js to goog.module and named requires, then a final clang-format of everything.

I have clang-format set to run on changed lines, which is something we as a team were doing before this big migration, but it sounds like that made things more confusing here. Since we're applying formatting to all files as we go, maybe I should turn it off for per-line changes and only run it in bulk on each file.

I did not do a bulk clang-format of all of the files i touched--I left that to be done on a per-file basis as we do updates.

…ersions

rachel-fenichel

I rebased and made requested changes. I did not try to fix require orders in any file that had not yet been converted to named requires and did not clang-format those files, since that will all be done in per-file PRs.

I did run clang-format on constants.js and internal_constants.js.

rachel-fenichel · 2021-07-21T22:40:19Z

core/internal_constants.js

+/**
+ * @fileoverview Module that provides constants for use inside Blockly. Do not
+ * use these constants outside of the core library.
+ * @author fenichel@google.com (Rachel Fenichel)
+ */


cpcallen · 2021-07-22T10:07:03Z

I have clang-format set to run on changed lines, which is something we as a team were doing before this big migration, but it sounds like that made things more confusing here.

Ordinarily I think that's a great idea (I have emacs configured to gofmt any time it saves a .go file)—but yes: where the file is going to be clang-formated in a later step I find it easier to review the non-formatting changes separately.

I did not do a bulk clang-format of all of the files i touched--I left that to be done on a per-file basis as we do updates.

Yes, sensible enough (and a good cause for me to pipe down about the format changes that did get made in these files, as I'd otherwise be complaining about them being left with overly-long lines). Can you confirm that what is left of constants.js is correctly formatted, since that is now migrated (and you didn't touch every line of it)?

cpcallen · 2021-07-22T10:15:38Z

(Oh, sorry: overnight my browser window had updated with your first reply but not the second one, and I forgot to hit reload. Thanks for confirming that the two migrated files are formatted!)

rachel-fenichel requested a review from a team as a code owner July 20, 2021 21:59

rachel-fenichel requested review from cpcallen and gonfunko and removed request for gonfunko July 20, 2021 21:59

github-actions bot added this to the 2021_q3_release milestone Jul 20, 2021

github-actions bot added the type: cleanup label Jul 20, 2021

rachel-fenichel assigned cpcallen Jul 20, 2021

cpcallen mentioned this pull request Jul 21, 2021

Break up and remove core/internal_constants.js and core/constants.js #5158

Open

cpcallen approved these changes Jul 21, 2021

View reviewed changes

cpcallen assigned rachel-fenichel and unassigned cpcallen Jul 21, 2021

rachel-fenichel added 8 commits July 21, 2021 15:33

Move constants to internal_constants.js and add aliases

c15f1e4

Rebuild

324c3c0

Replace references to SPRITE and SOUND_LIMIT with internalConstants v…

cc7a263

…ersions

Convert more names to use internalConstants

ad5836b

Refer to more constants through internalConstants

8b5f5f7

Replace references to constants with internalConstants

b3f837a

Remove extra requires of Blockly.constants

cf7a8b9

Reorder some requires and rebuild

12636da

rachel-fenichel force-pushed the update_constants branch from 9688dcc to 12636da Compare July 21, 2021 22:48

rachel-fenichel commented Jul 21, 2021

View reviewed changes

rachel-fenichel merged commit 6b05920 into RaspberryPiFoundation:goog_module Jul 21, 2021

rachel-fenichel deleted the update_constants branch July 21, 2021 22:52

This was referenced Jul 22, 2021

Migrate core/block_dragger.js to goog.module syntax #5171

Merged

Migrate core/flyout_horizontal.js to goog.module syntax #5190

Merged

maribethb mentioned this pull request Sep 28, 2021

Clean up constants/internalConstants aliases in blockly.js #5554

Open

Make constants.js have no side effects #5140

Make constants.js have no side effects #5140

Uh oh!

Conversation

rachel-fenichel commented Jul 20, 2021

The basics

The details

Resolves

Proposed Changes

Additional Information

Uh oh!

cpcallen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel commented Jul 21, 2021

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

rachel-fenichel Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpcallen commented Jul 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cpcallen left a comment •

edited

Loading

cpcallen commented Jul 22, 2021 •

edited

Loading