Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 15, 2019

Conversation

lddubeau
Copy link
Contributor

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.

Fixes ivogabe#563

The fix is actually the change in main.ts. The same change was applied to
project.ts though I've not found a way to create a test case that generates
there. (It may not be currently possible to generate such a case.) It just seems
that the two calls to parseJsonConfigFileContent should pass the whole path to
the config.
@lddubeau lddubeau force-pushed the fix/relative-config-path branch from c86db1c to 2551704 Compare February 14, 2019 18:55
@lddubeau
Copy link
Contributor Author

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
Copy link
Owner

ivogabe commented Feb 14, 2019

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 ivogabe merged commit ea22fb7 into ivogabe:master Feb 15, 2019
@ivogabe
Copy link
Owner

ivogabe commented Feb 15, 2019

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

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

Successfully merging this pull request may close these issues.

gulp-typescript doesn't resolve @types in outter node_modules
2 participants