Fix for #1992, along with chunk interface refactoring #1994
Conversation
…ser interop interfaces
guybedford
force-pushed
the
imports-listing-bug
branch
from
2727174
to
9bbd518
Compare
src/Chunk.ts
Outdated
| reexports?: ReexportSpecifier[]; | ||
| imports?: ImportSpecifier[]; | ||
| }[]; | ||
| }; |
There was a problem hiding this comment.
As far as I know, interfaces work syntactically like functions and classes in that trailing semicolons are treated like empty statements (in contrast to types).
There was a problem hiding this comment.
To be honest, I have no idea what the rules are so have just defaulted to using a semicolon everywhere. But you are probably right in that it is a declaration.
| @@ -97,7 +97,6 @@ export default class Chunk { | |||
| } | |||
| }; | |||
| dependencies: (ExternalModule | Chunk)[]; | |||
| externalModules: ExternalModule[]; | |||
There was a problem hiding this comment.
Awesome you could get rid of this! Really like this kind of change!
There was a problem hiding this comment.
Great, yes worth keeping the refactorings going!
src/Chunk.ts
Outdated
| @@ -592,10 +586,26 @@ export default class Chunk { | |||
| } | |||
|
|
|||
| let reexports = reexportDeclarations[dep.id]; | |||
| const isExternal = !!(<ExternalModule>dep).isExternal; | |||
There was a problem hiding this comment.
I would suggest to inline this constant. Also I think the boolean type-cast is rather unnecessary if we just write if ((<ExternalModule>dep).isExternal === true)
| dependencies.push({ | ||
| id: dep.id, | ||
| name: dep.name, | ||
| isChunk: !(<ExternalModule>dep).isExternal, | ||
| exportsNames, | ||
| exportsNamespace, | ||
| exportsDefault, | ||
| reexports, | ||
| imports | ||
| }); |
There was a problem hiding this comment.
As this is becoming bigger, I would suggest to extract the forEach loop into a private method getModuleDeclarationDependencies(reexportDeclarations). Also you convinced me that for-of-loops are really more efficient while usually being just as readable so we might switch to one of those 😉
Even better, we might also extract the top part of this function into a private method getReexportDeclarations() and call this directly from getModuleDeclarationDependencies() and get rid of this last parameter there. Apart from better structuring, this would also make it clear that there are no dependencies between these code blocks which should make it easier to make confident refactorings in the future.
There was a problem hiding this comment.
Unfortunately the scope of all variables in this function follows the whole function. See for example reexportDeclarations is referenced by the dependencies loop, and imports being referenced from above as well.
There was a problem hiding this comment.
No, this is exactly the kind of confusion I would like to avoid with this extraction. I was talking about the forEach loop starting at L.573. Except for the variables reexportDeclarations as an input and dependencies as an output, this loop is completely self-contained (as I tried out in my IDE). This is easily overlooked as long as it is just somewhere inside the larger function scope.
The same goes for the for-loop starting at L.553 which only produces reexportDeclarations.
So to repeat, my suggestion is:
- extract L.549-L.569 into a private method
getReexportDeclarations(). - extract L.571-L.610 into a private method
getModuleDeclarationDependencies()which itself callsgetReexportDeclarations()to get the re-exports.
So everything that remains in this function is a call to getModuleDeclarationDependencies() and the generation of exports which may or may not be extracted itself for symmetry reasons.
|
I may try and bring this work into the current default fix refactoring I'm doing as it may have some related parts - please hold off on merging for a little while till I can confirm! |
|
I've checked and this merges fine separately to the default naming fix PR, so this can be merged anytime. |
Resolves #1992.
The fix for #1992 was relatively straightforward here.
While I was at it I noticed
chunk.externalModulesis a kind of semi-public interface used by finalisers, which could easily get out of sync with further optimization, so I refactored the usage here to be entirely dependent on the data structure created by chunks for their boundaries instead of needing to reach into internals like this. It's not the prettiest of code, but it provides the encapsulation which is necessary to separate thinking about the input graph from thinking about the output graph.