fix(fields)!: Make Field.SKIP_SETUP a Symbol; remove class Sentinel
#6919
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
|
@btw17: I can't formally request a review from you, but I would like your comments if you have any. |
|
Addendum: this PR also removes the |
|
This is a late change but I agree with merging it into the upcoming release:
|
|
First off, I think making tldr, though, I believe Field plugin subclasses should be fine as long as they call Clarifying my intent in #6639:
// Before #6639
class Field<T> {
static readonly SKIP_SETUP = new Sentinel();
// ...
constructor(value: T|Sentinel /*, ... */) {
// ...
if (opt_value === Field.SKIP_SETUP) {
return;
}
}
// ...
}
// After #6639
class Field<T> {
static readonly SKIP_SETUP = new Sentinel();
static isSentinel<T>(value: T|Sentinel): value is Sentinel {
return value === Field.SKIP_SETUP;
}
// ...
constructor(value: T|Sentinel /*, ... */) {
// ...
if (Field.isSentinel(opt_value)) return;
}
// ...
} |
|
I think this is now awaiting formal approval by @rachel-fenichel. |
|
Please fix the PR title (no !) before merging. |
It was introduced during the ES5->ES6 phase of the migration, and it is almost certainly my fault, though I am no longer exactly sure why I suggested creating a
Actually, using
That's true, though any code that was likely to need
I think this was a very sensible choice on your part, though I also note that it may have made the type checker a little complacent about the possibility of a naughty developer passing in a |
|
(Previous comment edited due to being inadvertently submitted before complete. Check hopefully final version here. |
Field.SKIP_SETUP a Symbol; remove class SentinelField.SKIP_SETUP a Symbol; remove class Sentinel
We introduced the SKIP_SETUP sentinel value when converting Field and its subclasses to ES6 class syntax, because super must be called before any other code in a subclass constructor, breaking the previous mechanism where subclasses would set some properties before calling their superclass constructor. SKIP_SETUP was a singleton value of class Sentinel. Recently, in PR google#6639 @btw17 introduced the isSentinel type predicate to improve the typing of Field. Unfortunately, there were some aspects of this change that were not very elegant: - isSentinel was declared as a static method on Field (rather than on Sentinel itself). - It only checks against the specific value SKIP_SETUP, rather than checking if the argument is instanceof Sentinel (though perhaps this is for efficiency?) Additionally - as a result of the migration from ES6 to TS, and predating PR google#6639 - The signature for the Field constructor's first argument was typed T|Sentinel, with subclass constructors generally being <some type(s)>|Sentinel. This creates a small problem when attempting to port Fields from core to plugins, because Sentinel is not reexported by core/utils.ts (and therefore not from core/blockly.ts either). The latter problem could be solved simply by reexporting Sentinel, or by having plugin constructors not accept SKIP_SETUP (though this potentially makes them more difficult to subclass), but neither is particularly desirable. Instead, this PR proposes that we: - Make Field.SKIP_SETUP a (unique) Symbol. - Change the value argument to the Field constructor to accept T|typeof Field.SKIP_SETUP - where typeof Field.SKIP_SETUP is (like a literal type) a type that accepts just the single value SKIP_SETUP. - Remove the Sentinel class and core/utils/sentinel.ts. BREAKING CHANGE: - Removes Field.isSentinel - though this addition has not yet been published, so it can only break our own as-yet-unreleased code in samples. - Changes the type of Field.SKIP_SETUP and the first argument of the Field constructor from Sentinel to typeof SKIP_SETUP (a unique Symbol) - but given that Sentinel has never been exported this should not break any actual external code.
Field.SKIP_SETUP a Symbol; remove class SentinelField.SKIP_SETUP a Symbol; remove class Sentinel
The basics
npm run formatandnpm run lintThe details
Resolves
We introduced the
SKIP_SETUPsentinel value when convertingFieldand its subclasses to ES6classsyntax, becausesupermust be called before any other code in a subclass constructor, breaking the previous mechanism where subclasses would set some properties before calling their superclass constructor.SKIP_SETUPwas a singleton value of class Sentinel.Recently, in PR #6639 @btw17 introduced the
isSentineltype predicate to improve the typing ofField. Unfortunately, there were some aspects of this change that were not very elegant:isSentinelwas declared as astaticmethod onField(rather than onSentinelitself).SKIP_SETUP, rather than checking if the argument isinstanceof Sentinel(though perhaps this is for efficiency?)Additionally—as a result of the migration from ES6 to TS, and predating PR #6639—the signature for the
Fieldconstructor's first argument wasvalue: T|Sentinel, with subclass constructors generally being<some types>|Sentinel.This creates a small problem when attempting to port fields from core to plugins, because
Sentinelis not reexported bycore/utils.ts(and therefore not fromcore/blockly.tseither).Proposed Changes
The latter problem could be solved simply by reexporting
Sentinel, or by having plugin constructors not acceptSKIP_SETUP(though this potentially makes them more difficult to subclass), but neither is particularly desirable.Instead, this PR proposes that we:
Field.SKIP_SETUPaSymbol.Field.isSentinel.Fieldconstructor to acceptT|typeof Field.SKIP_SETUP—wheretypeof Field.SKIP_SETUPis (like a literal type) a type that accepts just the single valueSKIP_SETUP.Sentinelclass andcore/utils/sentinel.ts.Reason for Changes
To allow plug-in
Fieldsubclasses to use the sameSKIP_SETUPmechanism as in-core fields.Test Coverage
Passes
npm test. No changes to manual testing expected.Documentation
No updates beyond automatic updates of generated reference docs.
Additional Information
BREAKING CHANGE:
Removes
Field.isSentinelChanges the type of
Field.SKIP_SETUPand the first argument of theFieldconstructor fromSentineltotypeof SKIP_SETUP(a unique Symbol)Sentinelhas never been exported this should not break any actual external code.For the above-noted reasons these changes should not necessitate a major version bump.