Narrow down enum types #431
Conversation
|
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
|
|
||
| if (--remainder <= 0) { break; } | ||
| } | ||
| ({done, lineHasWhitespace, lineHasContent, content, offset, remainder, newlines} = this._checkCharacterBackward(char, charAttributes, seekTextNoteDetails)); |
There was a problem hiding this comment.
My comment is a bit late on this, but these sub-functions should probably just be modifying the seekTextNoteDetails value that is passed into it. As it stands right now, it's repeatedly constructing and destructuring a large structure in a high-bandwidth code-path, which is probably not great. Obviously this project doesn't have performance tests currently, so I don't have hard data to back this up, but I'd imagine that this is likely less efficient.
One of the reasons why this code path was originally somewhat messy was because it was a highly-used function. That's why it was doing the weird thing where it would assign all of the member fields to local variables first, and then restore them after: for (presumably) faster access in a loop. However, now that that effect is no longer being used, there is really no reason to do that, and instead the member fields should be used directly.
This is what it would look like:
https://github.com/toasted-nutbread/yomichan/commits/simplify-dom-text-scanner/
But I'm also not sure if there's a strong benefit of making that change vs just leaving the loop in the function. There's probably a lot of code locality and caching stuff that goes on in JS there, so it's hard to say what is the best way to do this in the long run without performance benchmarks.
This change narrows down the number type to an union of number literals to represent an enum. It also changes the if else matching pattern to switch statements to more closely match the enum matching patterns in other programming languages, as well as already existing code inside the project.
Part of the movement towards type safety.