refactor!: remove constants for internal constants

[alschmiedt]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide

The details

Resolves

Part of #5158

Proposed Changes

• Removes DRAG_* constants from internal_constants.js. Double checked with Rachel and these are left over from the days where we had a dragMode_ value on the workspace. I believe this was replaced by isDragging().
• Moves constants that we have previously instructed developers to change from internal_constants.js to constants.js.
• Moves Blockly.SPRITE to Blockly.constants.SPRITE since it is used in multiple places and it should (AFAICT) actually be constant and not settable.

Behavior Before Change

• Changes to the following values would not be reflected internally:
  Blockly.DRAG_RADIUS, Blockly.FLYOUT_DRAG_RADIUS, Blockly.SNAP_RADIUS, Blockly.CONNECTING_SNAP_RADIUS, Blockly.CURRENT_CONNECTION_PREFERENCE and Blockly.BUMP_DELAY are no longer on the Blockly object.

Behavior After Change

• Developers can change any of these values in config.js and the changed value will be used internally. For example:
  Blockly.config.dragRadius = 5
• Blockly.SPRITE -> Blockly.constants.SPRITE. [Edit: After some discussion, we decided to not make this SPRITE constant available externally. If you need this constant, please let us know by filing an issue.]

Reason for Changes

Some questions that came up before this and during this change that I thought I would document here.

Why remove them from the Blockly object?

• AFAICT from looking through our major partners libraries and other GitHub projects these values are not widely used.
• The fix is pretty simple (Blockly.X --> Blockly.config.X)
• If we leave these on Blockly we would need to add a setter to set the internal value. Since I don't believe we want to do this for all the values, we then end up with some variables on Blockly where the value can be changed and some where it can not. I think this is a pretty annoying/confusing bug for external developers to have to figure out.

[Edit] I was originally going to put these in constants.js since I didn't think the distinction that they were not technically constant was all that important to external developers. However, putting it in config allows us to have real constants internally and further tighten our public API (since it means that only a subset of these values will be able to be changed).

Why not add these to options?
I wrote a one pager on this, but the gist is:

• These are not widely used.
• Our options struct is very stable and I am not convinced these constants are.

Additional Information

DRAG_RADIUS: Used in Scrach/makecode
FLYOUT_DRAG_RADIUS: I don't see anyone changing this, but I still added this because it goes with DRAG_RADIUS.
SNAP_RADIUS: code.org, scratch/makecode, blockly games
CONNECTING_SNAP_RADIUS: code.org, scratch/makecode, blockly games
CURRENT_CONNECTION_PREFERENCE: Scratch
BUMP_DELAY: Scratch, app inventor

[rachel-fenichel]

The values in Blockly.constants are not mutable after this change--as I understand it, if you export a simple value (such as a number) it doesn't actually change if you reassign it. That's in contrast to exporting something like an array, where you can modify the object (add and remove) after exporting. You still need setters and getters inside constants.js to make it work.

Also, please add these to renamings.js.

[cpcallen]

The values in Blockly.constants are not mutable after this change--as I understand it, if you export a simple value (such as a number) it doesn't actually change if you reassign it.

This is probably fairly clear to everyone already, but just to make sure there is no misunderstanding:

At the moment our exports are all sort-of-mutable. If you do this:

const constants = goog.require('Blockly.constants');
constants.DRAG_RADIUS = /* new value */

then:

• The const DRAG_RADIUS in constants.js will of course not be updated:
  • It's constant, obviously.
    • The exported value is a copy of it.
    • Of course, the const itslef is never used anywhere, so it not being updated is moot.
  • But your update will be seen by other modules that import Blockly.constants without destructuring, because they get the same exports object.

This is contrary to the styleguide (which bans mutable exports) and probably not wise because I'm not sure that "they get the same exports object" is guaranteed. On the other hand, if you do this:

const {DRAG_RADIUS} = goog.require('Blockly.constants');

then you can't modify your own copy (because it's const!) and won't see any modifications made to the exports object by other modules.

I would recommend that, if the values being moved to constants.js are indeed configuration constants they be exported on an options object. Such an object could be defined in this module or in a module of its own (perhaps Blockly.config or .options), and the options object (only) be reexported in blockly.js as Blockly.config (or Blockly.options), just as we presently do for Blockly.Blocks etc.:

goog.module('Blockly.options');

const options = {
  /** ... */
  DRAG_RADIUS: /* ... */;

  // ...
};
exports.options = options;

(If we really want to reexport them as Blockly.constants.*, then we could just put all the constants in constants.js on the options object and reexport it as Blockly.constants.)

Adding get/set methods or get/set accessor properties would also work, but I think this is unnecessarily heavyweight for values that are not expected to be changed at runtime.

Tangential aside for the future, in case it affects decisions now:

I went down the rabbit hole of export mutability in ES Modules and TypesScript.

• It turns out that ES Modules do support mutable exports—sort of. In particular, if you declare

   export let x = /* … */;

  and then later mutate x, those mutations will be visible to any module that imported x:
  • If another module does import x from … then the locally-visible value of x will change in that module.
  • If another module does import * as someName from …, then the value of someName.x will change.

However:

• The mutability is one-way: neither x (in the first case) or someName.x (in the second case) can be modified.
  • import x from … causes x to behave as if it were declared const.
  • import * as someName from … causes someName.x to behave as if it were neither writable(*) nor configurable.
• It is forbidden by the styleguide, most probably at least in part because
• Closure Compiler does not fully support mutable exports

I also delved into TypeScript module export mutability. I think this is intended to be the same as for ES modules. Originally there was a bug but it seems to have been fixed and so the behaviour should be the same as for ES Modules provided you use import and not require to load the module.

(*) Interestingly, Object.getOwnPropertyDescriptor(someName) returns {writable: true, configurable: false, …}; evidently Module objects are exotic.

[alschmiedt]

@cpcallen I am not at all confident that this is correct. Would you mind taking a look?

[cpcallen]

Did we do a patch release? If so, then this should be .3 or whatever the last point release was, rather than .0, unless these changes will be included in the point release, in which case it should be whatever the last release not to include the changes was).

This question brings to light a less-than-great aspect of how I decided to version the renamings, because every time there is a point release in master, the corresponding entry in develop should be updated accordingly (unless the renamings were actually included in the release, in which case a new section should be started in the usual way).

It might have been better to label renamings with the first version they do appear in (rather than the last one they don't appear in), with the "current" in-progress release just labelled "IN PROGRESS" or something, and add a step to the quarterly release process to replace that with whatever version they are actually released with. Thoughts?

[alschmiedt]

Just to clarify. You are thinking about the scenario where I want to release a patch with my current change. So we would have to go back and add these values to 6.x instead of 7.x? And if we instead did the first release that they appear, we would just have to add them to 7.x? I think I might not be understanding correctly. It seems like in either case you have to know whether you are going to release a patch with your change and then place it in the correct location?

[cpcallen]

I am thinking about two scenarios:

• If a patch is done which releases none of the code that contains renamings, then at the moment the entry in renamings.js should be changed from:

    '7.20211209.0': {

  to

    '7.20211209.1': {

  (or in this case .2 before and .3 after, since I think we've already done a couple of patch releases), because these renamings will take effect after the patch release.

• If a patch is done which releases some/all of the renamings, then this file should instead be changed to:

That is how things work with the versioning scheme I originally proposed and which we have been using. But maybe it would be better to change it, such that instead it would be versioned like this:

  '7.20211209.1': {
    // Renamings that are released in 7.20211209.1.
  },
  'develop': {
    // Renamings that will not be released until after 7.20211209.1.
  },

Where develop would be replaced by the actual version number during the release process.

[cpcallen]

(In any case, this discussion is tangential to this PR, and should not cause it to be delayed.)

[alschmiedt]

Sorry I am just getting back to this. Ah that makes sense, thanks for clarifying. I think having 'develop' is a good idea and it should be pretty easy to automate swapping out 'develop' with the release number when we run npm run recompile.

[cpcallen]

PR LGTM, but see below for a question about whether we should consider changing how the renamings are versioned.

[cpcallen]

Did we do a patch release? If so, then this should be .3 or whatever the last point release was, rather than .0, unless these changes will be included in the point release, in which case it should be whatever the last release not to include the changes was).

This question brings to light a less-than-great aspect of how I decided to version the renamings, because every time there is a point release in master, the corresponding entry in develop should be updated accordingly (unless the renamings were actually included in the release, in which case a new section should be started in the usual way).

It might have been better to label renamings with the first version they do appear in (rather than the last one they don't appear in), with the "current" in-progress release just labelled "IN PROGRESS" or something, and add a step to the quarterly release process to replace that with whatever version they are actually released with. Thoughts?

[cpcallen]

Should this export be marked as deprecated, or deleted outright?

The entry in renamings.js suggests that we expect developers to use Blockly.constants.SPRITE rather than Blockly.SPRITE. I would not suggest creating a set accessor just to print a deprecation warning, but perhaps putting a comment about it so we remember to delete it in due course, if you don't want to do so now?

[alschmiedt]

Hmm, I think it is actually fine to delete now. I looked back through GitHub and didn't see anyone using it externally.

This makes me think though- do we actually want to keep internal_constants.js around for values like this? I think the original issue says we should remove all values from internal_constants, but I think it serves a different purpose than constants which are public and we expect developers to use.

[cpcallen]

That seems like a good idea to me: don't make public anything which doesn't need to be. An alternative might be to introduce a core/sprite.js (Blockly.sprite perhaps?) that is imported by the two modules which need these constants. (I had a look to see if there were any other shared dependencies which might be a suitable home; the only one that looked at all promising was core/positionable_helpers.js (Blockly.uiPosition), but that doesn't seem quite right.

Maybe @rachel-fenichel has an opinion about this?

[rachel-fenichel]

I'm fine with deleting it from exports now and eventually moving it to its own file (core/sprite.js is reasonable). We certainly don't need to keep internal constants around long-term--everything in it should eventually end up in a more logical place.

[cpcallen]

Not the cleanest PR history I've ever seen—but the final result still LGTM!

[alschmiedt]

ha, I squash and merge so I always clean up then.

[cpcallen]

ha, I squash and merge so I always clean up then.

It's actually nice to see PRs that haven't been forced-pushed to a curated-Instagram-feed level of polish: it helps remind one that other people's development histories are often as convoluted as one's own.