Skip to content

Normalize relative external paths. - #591

Merged
Rich-Harris merged 3 commits into
rollup:masterfrom
fatfisz:normalize-relative-external-paths
Apr 22, 2016
Merged

Normalize relative external paths.#591
Rich-Harris merged 3 commits into
rollup:masterfrom
fatfisz:normalize-relative-external-paths

Conversation

@fatfisz

@fatfisz fatfisz commented Apr 4, 2016

Copy link
Copy Markdown
Contributor

I've encountered a problem in such a scenario:

  • I had a path to a module declared as an external (so that the file would be imported later, not by rollup)
  • a file that was not in the same directory as the entry file was importing that module (../../module)

It turned out that the imported module in the bundle was still ../../module, while I expected it to be normalized with respect to, let's say, the entry file.

I investigated a bit and found out something scary: if a relative module is imported from many files in a nested structure, there will be more than one import in the resulting bundle.

For example, I get this:

var foo = _interopDefault(require('./foo'));
var foo$1 = _interopDefault(require('../foo'));
var foo$2 = _interopDefault(require('../../foo'));

var first = foo$1 + foo$2;
var main = foo + first;
module.exports = main;

instead of this:

var foo = _interopDefault(require('./foo.js'));

var first = foo + foo;
var main = foo + first;
module.exports = main;

This PR fixes that problem by normalizing the paths with respect to the resolved entry directory.

I probably did some terrible mistakes because I don't fully understand what's happening there. So please be gentle 😉

Comment thread src/Bundle.js
});

this.entry = unixizePath( options.entry );
this.entryId = null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to create this variable. Use this.entryModule.id instead.

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.

I tried to, but this.entryModule is null at this point. This is because it is still being fetched (and it won't be completed until all its dependencies are fetched too).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's too bad. I guess it's fine then.

@fatfisz

fatfisz commented Apr 8, 2016

Copy link
Copy Markdown
Contributor Author

I added another test case, thought of it today:

|--first
|  |-b.js
|  \-config.js
|-a.js
\-config.js

Let's say that for some reason we want both config.js files to be external. Then if a.js references config.js and first/b.js references first/config.js, only one of the config.js files will be required in the bundle (the other one will be completely ignored).

Of course my patch fixes this too.

@Victorystick Is there anything still holding us back from merging this? Can I help somehow?

@Victorystick

Copy link
Copy Markdown
Contributor

@fatfisz I think it's looking good. /cc @rollup/collaborators

@Rich-Harris
Rich-Harris merged commit 70e3f06 into rollup:master Apr 22, 2016
@Rich-Harris

Copy link
Copy Markdown
Contributor

LGTM! Thanks @fatfisz, sorry for the wait – going to merge a few other PRs then cut a new release shortly 👍

@fatfisz
fatfisz deleted the normalize-relative-external-paths branch April 22, 2016 20:37
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.

3 participants