Skip to content

Normalize dependency graph paths - Fix broken dependencies on Windows#286

Merged
johnnyreilly merged 1 commit intoTypeStrong:masterfrom
pzavolinsky:master
Oct 11, 2016
Merged

Normalize dependency graph paths - Fix broken dependencies on Windows#286
johnnyreilly merged 1 commit intoTypeStrong:masterfrom
pzavolinsky:master

Conversation

@pzavolinsky
Copy link
Copy Markdown

@pzavolinsky pzavolinsky commented Sep 21, 2016

instance.dependencyGraph gets created using a raw containingFile value:

var servicesHost = {
    // ...
    resolveModuleNames: (moduleNames: string[], containingFile: string) => {
        // ...
        instance.dependencyGraph[containingFile] = ...
        // ...
    }
    // ...
}

But is later accessed with a normalized path:

function loader(contents) {
    // ...
    var filePath = path.normalize(this.resourcePath);
    // ...
    let additionalDependencies = instance.dependencyGraph[filePath];
    // ...
}

This breaks every scenario where path.normalize is not identity.

For example, in Windows, you get:

containingFile: 'C:/project/dir/file.ts'

path.normalize(containingFile): 'C:\\project\\dir\\file.ts'

@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Oct 9, 2016

Hi @pzavolinsky,

could you refork and resubmit this PR please? I've just got CI cleaned so the tests pass but i can't requeue a build for your PR manually...

@pzavolinsky
Copy link
Copy Markdown
Author

pzavolinsky commented Oct 10, 2016

Hi @johnnyreilly , I've just rebased my fork against ts-loader/master. You should be able to merge without conflicts now.

@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Oct 10, 2016

Hi @pzavolinsky,

Thanks for submitting that - happily the tests are passing with your change in place. 😄 The change looks good.

Could you tell me how you discovered this issue? I'm guessing there was some kind of a problem that you encountered.

I ask as I'm wondering if it may be worth adding a test to ensure there are no future regressions.

@pzavolinsky
Copy link
Copy Markdown
Author

pzavolinsky commented Oct 10, 2016

Could you tell me how you discovered this issue? I'm guessing there was some kind of a problem that you encountered.

Sure, here it goes:

We have lots of files called types.ts that define just reusable types. ts-loader was not picking up changes in those files. After a bit of debugging I found the path normalization issue and that dependencyGraph lookup always failing.

There must be something special going on for types.ts files, because other files work just fine, so you might want to look into that too.

Hope this helps!

@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Oct 10, 2016

Thanks for the explanation @pzavolinsky. Very helpful. I'm not too sure if there's a good way to cover this with a test but I want to have a think first. Assuming I don't come with anything I'll plan to merge this.

Thanks again!

@pzavolinsky
Copy link
Copy Markdown
Author

pzavolinsky commented Oct 10, 2016

Great, let me know if you need anything else!

@johnnyreilly johnnyreilly merged commit 922d9bb into TypeStrong:master Oct 11, 2016
@johnnyreilly
Copy link
Copy Markdown
Member

johnnyreilly commented Oct 11, 2016

Awesome - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@pzavolinsky @johnnyreilly