search

ivogabe / gulp-typescript

A TypeScript compiler for gulp with incremental compilation support.
MIT License
831 stars 129 forks source link

Allow resolving @types that name libraries in node_modules directories above cwd #610

Closed lddubeau closed 5 years ago

lddubeau commented 5 years ago

This fixes #563.

At first I thought the issue was merely due to the characteristics of the paths in the various configuration files, but the key is really whether the libraries that the TypeScript compiler needs to find are below or above its cwd. The problem happens when the libraries are above cwd.

I don't think there is a safe way to manipulate the cwd of the gulp process that is used to run the whole test suite. It is possible to change it, but changing it for one gulp task will affect other gulp tasks running in parallel. So I've added another type of test to the suite. If a test contains a gulpfile.js then this test is also deemed an "exec test" and the top gulpfile.js will launch a new gulp subprocess on the test's gulpfile. This is a pass/fail test: if the subprocess ends without error that's a pass.

lddubeau commented 5 years ago

I've forced-pushed a new version of this PR. My first submission fixed the call in main.ts. Then I realized there was a call in project.ts that also needed to be taken care of.

The test case I've added is passes once the fix in main.ts is present, and fails if this fix is removed. Unfortunately, I have no way to create a test that will require the change in project.ts to pass. The code there does not depend on resolving modules listed in @types, and I don't know any other set of conditions that could be used to trigger a failure there.

(The earlier Travis failure seems spurious to me. When I checked I saw that Travis timed out the test while git was fetching submodules. The suite did not even get to run!)

ivogabe commented 5 years ago

Thanks @lddubeau for the PR and @FunkMonkey for the debugging!

The test case I've added is passes once the fix in main.ts is present, and fails if this fix is removed. Unfortunately, I have no way to create a test that will require the change in project.ts to pass. The code there does not depend on resolving modules listed in @types, and I don't know any other set of conditions that could be used to trigger a failure there.

The code in project.ts is used for the .src() interface. It does not resolve @types, so you'll not notice the bug there. I think it's better to apply the fix there as well like you did, for consistency.

Your changes look good, glancing at the diff. I'll test it tomorrow.

As for the failing CI, I'll ignore that. We probably need to stop testing for older versions of TS, but that's a different issue.

ivogabe commented 5 years ago

Thanks for the fix @lddubeau! Will do a new release in a few days