refactor: convert row.js and base.js to ES6 classes #5957
Conversation
|
I think removing that property assignment in the most recent commit is fine. It was added in #3740 to fix the bounding boxes of blocks, but I don't think it actually did that. I removed the assignment and bounding boxes still look fine:
|
There was a problem hiding this comment.
Looks good to me. I will leave final approval to @rachel-fenichel, once she has looked at the statementEdge question.
| } else if ( | ||
| Types.isPreviousConnection(elem) && elem instanceof Connection) { |
There was a problem hiding this comment.
I'm assuming that Types.isPreviousConnection(elem) implies that elem instanceof Connection and you're just helping the compiler figure that out, but if that's not so beware that this could result in a change of behaviour, where cases that would previously have matched this if clause could end up falling through and potentially matching later ones.
A slightly more conservative approach to adding a type check here would be:
} else if (Types.isPreviousConnection(elem)) {
if(!(elem instanceof Connection)) throw new TypeError(/*...*/);(Here and various other places.)
There was a problem hiding this comment.
I'm pretty sure that these instanceof checks shouldn't modify behavior, especially because we rely on properties of the checked class existing.
Honestly, I'm not sure why Types exists at all. It looks like it was meant to unify a bunch of ad-hoc "type" checks. But it seems like instanceof works just as well.
There was a problem hiding this comment.
Just bumping this to check your opinion before merging @cpcallen
The basics
The details
Resolves
Last of #5860
Proposed Changes
There were some places where extra properties were getting assigned to
Rowinstances. This moves them into WeakMaps instead because they are only used by certain subclass renderers.I also added a lot of
instanceofchecks for row elements to fix type errors. But I left theType.isX()calls as well. Not sure if we want to remove those or not.Should be no change in behavior 🤞
Reason for Changes
The long winding journey to TypeScript
Test Coverage
Sanity checked that all renderers look correct.
Tested on:
Documentation
N/A
Additional Information
N/A