Skip to content

Resolve modules, type reference directives in context of referenced file #27560

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

Merged
merged 14 commits into from
Oct 18, 2018

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Oct 4, 2018

This PR adds the ability to resolve module and type reference directives in the project direct file with its own options (eg project structure of A -> B -> C, while resolving modules/type reference directives in B we will use B's config file options to resolve it rather than using A's config file options)
Fixes #27200

@ajafff
Copy link
Contributor

ajafff commented Oct 8, 2018

Wow, there's a heck of a lot changes in this PR.
I'm not sure I understand why all of them are necessary. So maybe the approach presented below doesn't fulfil all requirements.

My approach would more closely align with the old implementation:

  • parse projectReferences recursively (like this PR does)
  • resolve imports and type references as if there were no project references at all
    • of course using the compilerOptions of the containing project of the current file
    • module resolver doesn't need to know about project references at all
  • map resolved path to declaration file of project reference if necessary (taking only direct references of the current containing project into account) like the old implementation
  • moduleResolutionCache depends on the current containing project's compilerOptions
    • maybe you could even reuse the parent's cache if the compilerOptions of a project reference doesn't differ in options that affect module resolution

That way you could get rid of all changes in moduleNameResolver.ts and probably all of the public API changes.

@sheetalkamat
Copy link
Member Author

module resolver passes current project reference so it can use the compiler options of the containing project. (instead of options it passes Project Reference since it is more future proof.) The changes in module resolver are needed so it can use compiler options of containing project instead of project it is building.

@ajafff
Copy link
Contributor

ajafff commented Oct 8, 2018

I agree that it makes sense to pass compilerOptions to CompilerHost.resolveModuleNames and CompilerHost.resolveTypeReferenceDirectives. But I don't think the module resolver should know about the concept of project references.

@sheetalkamat
Copy link
Member Author

I don't think just passing compilerOptions is good idea since the current way (passing containing projectReference which sourceFile of config file apart from its commandline) lets host do custom action eg. your build expecially in watch mode or editor could reuse the resolution from that cache - this is not yet part of this change but its intended for future use.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't looked at it in detail yet, but the approach made me want to see a test of a node module resolution project referencing a non-node one and vice versa (and ensuring the, eg, if a classic project referenced a node project, node module identifiers resolve in the declarations representing the dependent project).

@weswigham weswigham self-assigned this Oct 11, 2018
@sheetalkamat
Copy link
Member Author

I can add classic resolution in referenced project test case but note that in the current one I verify path mapping changes in referenced project are handled correctly.

@weswigham
Copy link
Member

I specifically am calling out a change in module resolution scheme because they use differing underlying resolvers.

@@ -4959,13 +4963,13 @@ namespace ts {
* If resolveModuleNames is implemented then implementation for members from ModuleResolutionHost can be just
* 'throw new Error("NotImplemented")'
*/
resolveModuleNames?(moduleNames: string[], containingFile: string, reusedNames?: string[]): (ResolvedModule | undefined)[];
resolveModuleNames?(moduleNames: string[], containingFile: string, reusedNames?: string[], redirectedReference?: ResolvedProjectReference): (ResolvedModule | undefined)[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when resolving module names in the root project, there will be no ResolvedProjectReference. Therefore a custom CompilerHost has no access to the CompilerOptions of that project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not changed since it was original API as well. In general host already knows root compiler options. Eg. https://github.com/TypeStrong/ts-loader/blob/master/src/servicesHost.ts#L498

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general host already knows root compiler options.

Correct, although that only works when building exactly one project with that host. The current implementation of tsbuild.ts (I know it's still considered internal) uses the same SolutionBuilderHost to build all projects. That host cannot know about the compilerOptions of all projects. And since tsbuild can even handle multiple tsconfig.json files at once, there's no way the host could ever know the correct compilerOptions.

Copy link
Member Author

@sheetalkamat sheetalkamat Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to change that. (the default compilerHost is for specific compilerOptions) Infact we want to change tsbuilder to create compiler host for each program separaterly but that's a todo for little later.

@sheetalkamat
Copy link
Member Author

@weswigham @rbuckton Please review.

@sheetalkamat sheetalkamat merged commit 91bb32a into master Oct 18, 2018
@sheetalkamat sheetalkamat deleted the transitiveReferences branch October 18, 2018 20:03
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.

Should project references check for transitive references to do the declaration file include check
4 participants