Fix namespace reexports bugs - #2157
Conversation
lukastaegert
left a comment
There was a problem hiding this comment.
Very nice, thank you for looking into this and fixing it, I think I may have stumbled over this myself at some point. There is just one thing where I would have liked another test, otherwise this is good to go!
|
|
||
| let hasReexports = false; | ||
| for (const importName in this.reexports) { | ||
| if (importName in this.exports) continue; |
There was a problem hiding this comment.
Removing this line does not produce any red tests. What is the condition in which this can happen and can we add this to a test?
There was a problem hiding this comment.
You're completely right - this check is unnecessary as exports shadowing reexports throw an early error.
| export interface ImportDescription { | ||
| source: string; | ||
| specifier: ImportSpecifier | ImportNamespaceSpecifier | ImportDefaultSpecifier; | ||
| start: number; |
There was a problem hiding this comment.
This removes some unnecessary API 👍
0141188 to
46afa80
Compare
|
@guybedford Excuse me to ping you, I've found this other, maybe related bug I'm using latest rollup and a default export like: https://github.com/caub/color-tf/blob/master/src/hsl2hsv.js |
This fixes #2156, which actually turned out to expose quite a few separate bugs:
undefinedwhere a module specifier string name should be.The fix for (1) is that one-liner, while the fix for (2) is to have namespaces being included populate an import for all reexported modules, and update the linkage graph to ensure this is handled for modules and chunks.
In the process of working on (2) I discovered and resolved two more bugs:
Hashes were being cached between different renders, so weren't being calculated correctly for module formats other than the "es" format being rendered first. The fix here is to clear the
chunk.renderHashvalue on each render.Generated imported module names for chunks are not currently being deshadowed -
import x from 'chunk2'would not properly deshadow against a localxvariable. This fix for this was to change theisExternalcheck on this deshadowing ofmodule.nameto include both externals and other chunk names (src/Chunk.ts:513).