Refactor MultiCompilerHost #55
Conversation
|
|
If you’d like to contribute, please comment on an issue or open a new one. Thanks! |
|
Right for changes that aren't refactors or larger changes I usually open an issue first but for smaller refactors I don't really see the point, since the changes can just as well be discussed in the PR. I was trying to make sense of the MultiCompilerHost implementation and found the code repetitive and hard to read, so I ended up doing this refactor (which I tried to explain in the last commit message). |
|
Sorry to jump the gun a bit on closing this. When I saw a PR with zero description I assumed there was very little chance this would be valuable. I like this over all. I’m not a huge fan of classes, and the switch from closures to classes feels a bit arbitrary. I wouldn’t unconditionally reject classes, but |
The logic of the MultiCompilerHost almost only operated on one CompilerHost plus respective CompilerOptions and caches, so it makes sense to encapsulate this logic for one instance of these properties, instead of requiring each of these properities to be a Record and having to index them all the time. The resulting code is less repetitive and more readable.
not-my-profile
force-pushed
the
refactor-core
branch
from
0590439 to
28f8c43
Compare
|
Thanks for taking a look at the changes. I just rebased the PR. I agree about the two ugly points you spotted, but they are not due to the switch to classes. I just fixed both of them by using arrow functions (which you apparently can also do in a class). btw if you merge this I'd appreciate the individual commits to be preserved to get more readable diffs in the git history. |
| class TraceCollector { | ||
| private traces: string[] = []; | ||
|
|
||
| trace = (message: string) => { | ||
| this.traces.push(message); | ||
| }; |
There was a problem hiding this comment.
I honestly didn't know that you could use an arrow function like this in a class to avoid having to bind the function outside but it works.
There was a problem hiding this comment.
Methods are declared on the class prototype so they can be shared between all instances. This is a class field, which is initialized on each instance during construction (equivalent to doing Object.defineProperty(this, "trace", ...) in the constructor). This has a perf/memory cost as compared to a true method, but of course, the cost is equivalent to every other way of accomplishing what we’re trying to accomplish here—.bind also creates a new copy of the function; it was just happening at a different time and place. There’s no way around creating a new function identity for each CompilerHost["trace"]. (As a piece of trivia, this was a common trick for React devs in the days of class components for event handlers, so you could do <button onClick={this.onClick}> instead of this.onClick.bind(this).) This still has a bit of a weird smell to me since you have to recognize which functions need this weird syntactic trick and which don’t. On the upside though, it’s a bit more efficient since at least read and clear are shared between all instances. It’s mostly a personal preference. I don’t love it but I appreciate the contribution so I’ll merge it 😛
2 participants
No description provided.