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

Import separator fix in Windows #59

Closed
wants to merge 4 commits into from

Conversation

GiedriusGrabauskas
Copy link

Issue: #52

I try to debug at line index.ts#L152

And that's happening because rollup function resolveId now gives importer param with OS path separators.
Changed in 0.33.1 with this PR: rollup/rollup#760

Before in Windows:

importer = "D:/test/r/test/sample/overriding-typescript/main.ts"

Now in Windows:

importer = "D:\\test\\r\\test\\sample\\overriding-typescript\\main.ts"

But Typescript nodeModuleNameResolver accept only with normal separator (/).

@GiedriusGrabauskas
Copy link
Author

@Victorystick Your tests failed in newest rollup vesion.

@TrySound
Copy link
Member

TrySound commented Aug 2, 2016

@GiedriusGrabauskas Can you write test which fails without your change?

@GiedriusGrabauskas
Copy link
Author

Test reports diagnostics and throws if errors occur during transpilation failed with new rollup version.
Details: https://travis-ci.org/rollup/rollup-plugin-typescript/jobs/149183785

@TrySound
Copy link
Member

TrySound commented Aug 2, 2016

Okay, then can you fix these tests?

@ghost
Copy link

ghost commented Aug 2, 2016

@GiedriusGrabauskas if you try to roll back the changes done in version 0.33.1 it will almost work. However some changes in version . 0.33.0 will make this fail to.

Rollup v. 0.32.4 is the latest working rollup version without this issues.

I reported this to @TrySound earlier

@ghost
Copy link

ghost commented Aug 2, 2016

@GiedriusGrabauskas You could try to do process.platform === 'win32' to check if it's windows, and if it is you can use a regExp - x.replace('/\\/g', '/') - to normalize the path.

This is an expensive operation and will reduce perf with around 3 - 4 ms on huge files.

I guess this should have been done in the Rollup core, but I got reported back there isn't a bug there.

@TrySound This error is on Travis, and Travis isn't run in a Windows environment. So it has to be more then Windows triggering this.

@TrySound
Copy link
Member

TrySound commented Aug 3, 2016

@GiedriusGrabauskas I added appveyor ci.

@TrySound
Copy link
Member

TrySound commented Aug 3, 2016

@KFlash It's not rollup responsibility if typescript don't understand environment path type. And 3-4ms is very opinionated. It's not the case where we should think about performance. Moreover resolveId is cachable now.

@ghost
Copy link

ghost commented Aug 3, 2016

@TrySound You need to update the dependencies and devDependencies in the repo to trigger the error.

@GiedriusGrabauskas
Copy link
Author

I can't pass the tests on Windows, because rollup-plugin-typescript using [email protected] from npm, to rollup itself. But 0.7.7 has a critical error...

 - Fixed `reports diagnostics and throws if errors occur during transpilation` test.
 - Removed file path duplicate from error message.
@TrySound
Copy link
Member

TrySound commented Aug 3, 2016

X) That's a dilemma. Can you remove types and use buble transpiller for this? Temporary of course. In the next release we will fix that. Or not :)
https://www.npmjs.com/package/rollup-plugin-buble

@TrySound
Copy link
Member

TrySound commented Aug 3, 2016

Thanks, guys!

@TrySound TrySound closed this Aug 3, 2016
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.

2 participants