Skip to content

Fix namespace reexports bugs - #2157

Merged
lukastaegert merged 3 commits into
masterfrom
namespace-reexports
Apr 29, 2018
Merged

Fix namespace reexports bugs#2157
lukastaegert merged 3 commits into
masterfrom
namespace-reexports

Conversation

@guybedford

@guybedford guybedford commented Apr 25, 2018

Copy link
Copy Markdown
Contributor

This fixes #2156, which actually turned out to expose quite a few separate bugs:

  1. src/Chunk.ts:193 needs to trace the full export through reexports, not just the variable trace. This stops a bug where Rollup would render undefined where a module specifier string name should be.
  2. Namespace object reexports not working at all in Rollup, which seems to have always been an issue - https://rollupjs.org/repl?version=0.58.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMColMjBhcyUyMG0lMjBmcm9tJTIwJy4lMkZtYXRocy5qcyclM0IlNUNuJTVDbmNvbnNvbGUubG9nKE9iamVjdC5rZXlzKG0pKSUzQiUyMiU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJtYXRocy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjAlN0IlMjB4JTIwJTdEJTIwZnJvbSUyMCdleHRlcm5hbDInJTNCJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmNqcyUyMiUyQyUyMm5hbWUlMjIlM0ElMjJteUJ1bmRsZSUyMiUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCU3RCUyQyUyMmV4YW1wbGUlMjIlM0FudWxsJTdE

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:

  1. 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.renderHash value on each render.

  2. Generated imported module names for chunks are not currently being deshadowed - import x from 'chunk2' would not properly deshadow against a local x variable. This fix for this was to change the isExternal check on this deshadowing of module.name to include both externals and other chunk names (src/Chunk.ts:513).

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment thread src/Module.ts Outdated

let hasReexports = false;
for (const importName in this.reexports) {
if (importName in this.exports) continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're completely right - this check is unnecessary as exports shadowing reexports throw an early error.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread src/Module.ts
export interface ImportDescription {
source: string;
specifier: ImportSpecifier | ImportNamespaceSpecifier | ImportDefaultSpecifier;
start: number;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes some unnecessary API 👍

@lukastaegert lukastaegert added this to the 0.59.0 milestone Apr 29, 2018
@lukastaegert
lukastaegert merged commit 46afa80 into master Apr 29, 2018
@lukastaegert
lukastaegert deleted the namespace-reexports branch April 29, 2018 21:42
caub added a commit to caub/color-tf that referenced this pull request Apr 30, 2018
caub added a commit to caub/color-tf that referenced this pull request Apr 30, 2018
caub added a commit to caub/color-tf that referenced this pull request Apr 30, 2018
@caub

caub commented May 4, 2018

Copy link
Copy Markdown

@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
becomes this as cjs: https://unpkg.com/color-tf@5.0.2/hsl2hsv.js, it looks fine, but maybe exports.default = .. was more expected as the bug arises in proxy.js where I import this file https://unpkg.com/color-tf@5.0.2/proxy.js and where default is used in libs, which is undefined

@guybedford

Copy link
Copy Markdown
Contributor Author

@caub thanks, yes that's a bug - I've created #2176 to track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Code splitting not working with default export/import

3 participants