-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
@Victorystick Your tests failed in newest |
@GiedriusGrabauskas Can you write test which fails without your change? |
Test |
Okay, then can you fix these tests? |
@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 |
@GiedriusGrabauskas You could try to do 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. |
@GiedriusGrabauskas I added appveyor ci. |
@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. |
@TrySound You need to update the dependencies and devDependencies in the repo to trigger the error. |
I can't pass the tests on Windows, because |
- Fixed `reports diagnostics and throws if errors occur during transpilation` test. - Removed file path duplicate from error message.
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 :) |
db99526
to
529412e
Compare
Thanks, guys! |
Issue: #52
I try to debug at line index.ts#L152
And that's happening because rollup function
resolveId
now givesimporter
param with OS path separators.Changed in 0.33.1 with this PR: rollup/rollup#760
Before in Windows:
Now in Windows:
But Typescript
nodeModuleNameResolver
accept only with normal separator (/
).