Fix #74 and refactor name tracing mechanism #78
Conversation
| this.assumedGlobals[ name ] = true; | ||
| // TODO is this necessary in the ES6 case? | ||
| let name = makeLegalIdentifier( module.suggestedNames['*'] || module.suggestedNames.default || module.id ); | ||
| module.name = getSafeName( name ); |
There was a problem hiding this comment.
It isn't necessary in the ES6 case, but if we always do it, taking the needs of all module formats into account, we only have to deconflict once. Might as well do it in Bundle.build.
There was a problem hiding this comment.
👍 that makes sense
|
I'll try to integrate some key points from BundleScopes (which in truth got more complicated than I'd hoped) into the current master. There are some issues I think could be solved by doing so. |
| @@ -453,4 +464,55 @@ export default class Bundle { | |||
|
|
|||
| return ordered; | |||
| } | |||
|
|
|||
| trace ( module, localName, es6 ) { | |||
| const importDeclaration = module.imports[ localName ]; | |||
There was a problem hiding this comment.
This has bothered me a bit. module.imports is used for both imports and re-exports. The linking (dare I say, binding?) I'm proposing in #71, avoids this issue. Consider:
export { foo as bar } from './foo1';
import foo from './foo2';
export function baz () {
return foo(4, []);
}; Two identifiers foo are imported. Which one is in module.imports? Can't this cause a lot of issues down the road?
There was a problem hiding this comment.
Hmm... that's true, re-exports aren't supposed to create a binding in the local scope. I think it could be solved fairly easily by having a module.reexports alongside module.imports and module.exports, and using that in bundle.trace() and module.mark(), but that's just speculation at this point
superceded by #79
This started out as a straightforward attempt at fixing #74 and turned into a far more comprehensive chunk of work.
@Victorystick as I hadn't quite got my head round #71, I started from the existing codebase, but there's now a fair bit of overlap between this and #71, insofar as renaming things is now a bundle-level concern – handled by
bundle.trace()rather thanmodule.getCanonicalName(), which was a source of pain and no longer exists. Would be keen to get your thoughts. This is a bit simpler than #71 (only adds ~5 LOC excluding tests, net, and doesn't introduce any new classes), but I want to make sure that I'm not missing a load of necessary stuff from #71 or otherwise stomping on your work!There's probably a bit of dust that still needs clearing up, as after any largish refactor