Conversation
| Object.keys(this._parentRels).forEach((child) => { | ||
| if (base.parentRels[child]) { | ||
| base.parentRels[child].forEach(function(id) { | ||
| base.parentRels[child].forEach((id) => { |
There was a problem hiding this comment.
this is not a stylistic change, it's required to simplify how this is bound. Now it's clear that this refers to the class itself, instead of relying on Array.prototype.forEach's second argument
There was a problem hiding this comment.
Good point! That also makes it more intuitive to read.
But now the second argument of the .forEach (here on line 186) is actually not required anymore, and should be omitted, isn't it? I changed those in c3d07e8
PS: honestly, all those .forEach in this file seem to be a little unnecessary and could probably be just simple for loops now (especially those Object.keys(o).forEach(k => …) ones would be much clearer as for (const k in o) { … } in my opinion. But let's keep this PR minimal and simplify/refactor the code at a later point.
|
|
||
| const prototype = { | ||
| type: 'node', | ||
| type: /** @type {'node'} */ ('node'), |
There was a problem hiding this comment.
we define type as a constant value, instead of an abitrary string. This allows us to write code like this:
if (entity.type === 'node') {
// now the compiler knows that `entity` is a node,
// so properties like `entity.loc` are available, but `entity.members` is not available.
}
I mean, this particular class is not instantiated in too many places, so it wouldn't be that bad, would it? Also, if our long term plan is to eventually migrate every module over to ts it will end up being a mass update anyway. So, maybe refactoring this to the regular ES instantiation is probably better? FYI: I'm busy for the rest of this week, so I'll only have time for a proper review early next week. |
|
there are 4 classes which this problem affects:
all other classes are instantiated in <5 files, so those are irrelevant. 99% of these instantiations are in the unit tests. |
|
Sounds like one commit with the helper function workaround for the general refactoring ... And then one rename commit that hides the rev ... might be a good idea to have a clean setup for both, diffing and the code. |
|
this should be unblocked now as #12151 is merged |
|
awesome, this is now significantly simpler |
this is the first PR to migrate a core class to TS. I've tried to minimize the diff as much as possible.
click to view the outdated comment about the `new` operator
It raises a tricky question:
ES classes must be instantiated with the
newoperator (new coreGraph()instead ofcoreGraph()). That gives us two options:coreGraph(withnew coreGraph(, but I think we try to avoid mass updates?new. Then we don't have to update any other files.This PR includes option 2 as seperate commit. It's a bit odd, but i can't think of any other solution.
RapiD chose option 1. Thanks to git-blame-ignore-revs, it's possible to hide the mass-update commit from the git history (locally and on GitHub).
Not sure which approach is better. But it's a problem that we'll need to tackle somehow