search

rollup / rollup-plugin-commonjs

This module has moved and is now available at @rollup/plugin-commonjs / https://github.com/rollup/plugins/blob/master/packages/commonjs
MIT License
501 stars 126 forks source link

Normalize ids before looking up in named export map #406

Closed bterlson closed 5 years ago

bterlson commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

lukastaegert commented 5 years ago

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 commented 5 years ago

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

lukastaegert commented 5 years ago

Would this be a breaking change or a bug fix?

bterlson commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

Thanks, then I'll make it a minor.