Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Normalize ids before looking up in named export map #406

Merged
merged 4 commits into from Aug 27, 2019

Conversation

bterlson
Copy link
Contributor

This fixes an issue frequently hit when using the typescript rollup plugin. Because TypeScript always returns posix paths, but node resolve will normalize on a per-platform basis, comparisons must be carried out on normalized forms. This PR fixes some missing normalization and obviates rollup-plugin-typescript2's rollupCommonJSResolveHack option (/cc @ezolenko), and fixes #177 and probably a few others as well.

@bterlson
Copy link
Contributor Author

Ok, I need a better way to test this on Linux. I guess paths on linux must always be posix paths else things break.

@shellscape
Copy link
Contributor

I'd recommend stealing the CI config from rollup/rollup. We made some pretty solid containers that it uses.

src/index.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
test/test.js Outdated Show resolved Hide resolved
@lukastaegert
Copy link
Member

Ok, I need a better way to test this on Linux. I guess paths on linux must always be posix paths else things break

Definitely, see my comment.

@bterlson
Copy link
Contributor Author

@lukastaegert the latter approach seemed best actually. I've also updated the comments to be more clear, see if you like them now 😀


// Note: customNamedExport's keys must be normalized file paths.
// resolve and nodeResolveSync both return normalized file paths
// so no additional normalization is necessary.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm cool with this, this is educational and will help people not familiar with the code 👍

@lukastaegert
Copy link
Member

Would this be a breaking change or a bug fix?

@bterlson
Copy link
Contributor Author

@lukastaegert Sorry I missed your message. This should just be a bug fix that causes more things to work. It impacts e.g. rollup-plugin-typescript2, but only because the hack is no longer necessary (but doesn't do any harm).

@ezolenko
Copy link

Actually I think it will break people who are currently using rollupCommonJSResolveHack in rollup-plugin-typescript2 -- what that option does is makes rpt2 use resolve package to get the path, which will use native OS format. If this plugin switches to POSIX path, namedExports will be out of sync again, and people will have unset that option.

Should be a minor version at least.

@lukastaegert
Copy link
Member

Thanks, then I'll make it a minor.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

namedExports is using non-normalized resolved module path as module id
4 participants