Have module resolution be independend of --allowJs and --jsx flags · Pull Request #11704 · microsoft/TypeScript · GitHub

…and don't have moduleNameResolver responsible for omitting files based on compiler options.

This PR has relatively little observable effect, but gives some better error messages if a module would have been resolved to a tsx file but you forgot the "--jsx" flag.

The API should be backwards compatible as the new ResolvedModule data structure still contains the resolvedFileName field, and we have a method convertResolvedModuleFromHost which adds the new resolvedTsFileName and resolvedJsFileName properties if necessary.

This also technically causes breaking change in the module resolution. Previously we would ignore ".tsx" files without "--jsx", but now we will resolve to that module and report an error in the checker.

Also, the secondary lookup in resolvedTypeReferenceDirective now only looks for .d.ts files, as the documentation on ResolvedTypeReferenceDirective seems to indicate that that's all we should be returning. I'm also wondering if loadModuleFromNodeModulesWorker should do the same when searching through @types, and same for loadModuleFromNodeModulesAtTypes.

I will redo #11446 after this is in.

Vladimir Matveev (@vladima) brought up the point that we might want to consider getting rid of shims. That won't be necessary for this PR, as I haven't changed the resolvedModuleName method of CoreServicesShimObject. This method is public on the class, but the class isn't exported and is only used as an implementation of the CoreServicesShim interface, which doesn't define that method. Pinging Mohamed Hegazy (@mhegazy) too.

…moduleNameResolver responsible for omitting files based on compiler options

this should stay optional. otherwise we break other API

i do not understand why we need this. the two new properties are optional, and we have logic to handle them being missing. so do not see the value here.

mm. do you really need that check? can we check when we create them instead?

nit. i would rather we just use module.resolvedTsFileName || module.resolvedJsFileName for the module, and have this function just to report errors. this avoids the complexity of checking if the return type is string or not.

Vladimir Matveev (@vladima) can you take a look.

… avoid using resolution.resolvedFileName if the diagnostic is defined.

…e` and `resolvedJsFileName` optional properties (but still automatically infer one of them to supply if the host supplied neither)

why removing @internal?

I don' think this function should be here given that this file is part of core compiler bits and LSHost is specific to server

…solvedFileName` and `extension`. Also, change search order to do all TS searching before searching for JS.

Extenstion.TS is Zero so this will fail. probably want to check for undefined here.

optional?

If this is optional we will need fallback logic on every access. I thought it was easier to just perform the conversion when getting it from an external source. This would mean that older clients won't compile against the new compiler, but will still run.
We might choose to not have this property as it is techincally redundant, but it's neater to have this property and use switch statements than to use if (fileExtensionIs(resolved.resolvedFileName, ".ts")) { ... } else if (fileExtensionIs(resolved.resolvedFileName, ".tsx")) { ... } all over.

just a couple of API comments

Return both ts and js results from module resolution, and don't have …

b557211

…moduleNameResolver responsible for omitting files based on compiler options

ghost assigned Vladimir Matveev (vladima) Oct 18, 2016

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Oct 18, 2016

ghost assigned Mohamed Hegazy (mhegazy) Oct 18, 2016

Mohamed Hegazy (mhegazy) reviewed Oct 18, 2016

View reviewed changes

Andy Hanson added 3 commits October 18, 2016 14:22

Instead of getResolutionOrDiagnostic, use getResolutionDiagnostic and…

7c53a1d

… avoid using resolution.resolvedFileName if the diagnostic is defined.

Merge branch 'master' into refactor_module_resolution

1d201c1

Remove "ResolvedModuleFromHost" type and just make `resolvedTsFileNam…

df20cf3

…e` and `resolvedJsFileName` optional properties (but still automatically infer one of them to supply if the host supplied neither)

Vladimir Matveev (vladima) reviewed Oct 24, 2016

View reviewed changes

Andy Hanson added 4 commits October 25, 2016 08:02

Move most of resolveModuleNameForLsHost to lsHost

dc99355

Don't require resolvedTsFileName and resolvedJsFileName, just `re…

07bb258

…solvedFileName` and `extension`. Also, change search order to do all TS searching before searching for JS.

Merge branch 'master' into refactor_module_resolution

9130fbd

Move helper functions to core (fix build)

718d57f

Vladimir Matveev (vladima) approved these changes Oct 26, 2016

View reviewed changes

Mohamed Hegazy (mhegazy) reviewed Oct 26, 2016

View reviewed changes

Mohamed Hegazy (mhegazy) approved these changes Oct 26, 2016

View reviewed changes

ghost merged commit b5ba315 into master Oct 27, 2016

ghost deleted the refactor_module_resolution branch October 27, 2016 13:03

ghost changed the title Return both ts and js results from module resolution Have module resolution be independend of --allowJs and --jsx flags Oct 27, 2016

This was referenced Oct 27, 2016

Allow untyped imports #11889

Merged

Make extension property of ResolvedModule optional #11899

Merged

Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018

TypeScript Bot (typescript-bot) unassigned Vladimir Matveev (vladima) Jun 19, 2018

This pull request was closed.

                        * - don't use tripleslash references
                        */
-                      isExternalLibraryImport?: boolean;
+                      isExternalLibraryImport: boolean;

+                   * in which case they will be inferred from the file extension.
+                   * Prefer to return a full ResolvedModule.
+                   */
+                  export type ResolvedModuleFromHost = { resolvedFileName: string; isExternalLibraryImport: boolean } | ResolvedModule;

+                   * Extracts the file name from a ResolvedModule, or returns a DiagnosticMessage if we are not set up to use that kind of file.
+                   * The DiagnosticMessage's parameters are the imported module name, and the filename it resolved to.
+                   */
+                  export function getResolutionOrDiagnostic(options: CompilerOptions, { resolvedTsFileName: ts, resolvedJsFileName: js }: ResolvedModule): string | { file: string, diag: DiagnosticMessage } {

-                  /* @internal */
-                  export function trace(host: ModuleResolutionHost, message: DiagnosticMessage, ...args: any[]): void;
-                  export function trace(host: ModuleResolutionHost, message: DiagnosticMessage): void {
+                  function trace(host: ModuleResolutionHost, message: DiagnosticMessage, ...args: any[]): void;

-                          resolveModuleNamesWorker = (moduleNames, containingFile) => host.resolveModuleNames(moduleNames, containingFile);
+                          resolveModuleNamesWorker = (moduleNames, containingFile) => host.resolveModuleNames(moduleNames, containingFile).map(resolved => {
+                              // An older host may have omitted extension, in which case we should infer it from the file extension of resolvedFileName.
+                              if (!resolved || resolved.extension) {

Have module resolution be independend of --allowJs and --jsx flags · Pull Request #11704 · microsoft/TypeScript · GitHub

Uh oh!

Have module resolution be independend of --allowJs and --jsx flags - #11704

Have module resolution be independend of --allowJs and --jsx flags#11704
8 commits merged into
masterfrom
refactor_module_resolution

Deleted user (ghost) commented Oct 18, 2016 •
edited by ghost
Loading

Uh oh!

Uh oh!

Mohamed Hegazy (mhegazy) Oct 18, 2016

Uh oh!

Mohamed Hegazy (mhegazy) Oct 18, 2016

Uh oh!

Mohamed Hegazy (mhegazy) Oct 18, 2016

Uh oh!

Mohamed Hegazy (mhegazy) Oct 18, 2016

Uh oh!

Mohamed Hegazy (mhegazy) commented Oct 18, 2016

Uh oh!

Vladimir Matveev (vladima) Oct 18, 2016

Uh oh!

Vladimir Matveev (vladima) Oct 18, 2016

Uh oh!

Mohamed Hegazy (mhegazy) Oct 26, 2016

Uh oh!

Mohamed Hegazy (mhegazy) Oct 26, 2016

Uh oh!

Deleted user (ghost) Oct 26, 2016

Uh oh!

Mohamed Hegazy (mhegazy) left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

+                          return undefined;
+                      }
+                      // `resolvedTsFileName` and `resolvedJsFileName` should be present as properties even if undefined.
+                      else if ("resolvedTsFileName" in resolved) {

+                  /** LsHost uses a global cache of automatically-installed typings to help it resolve modules. */
+                  /* @internal */
+                  export function resolveModuleNameForLsHost(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, globalCache: string | undefined, projectName: string): ResolvedModuleWithFailedLookupLocations {

-                      /*
-                       * Denotes if 'resolvedFileName' is isExternalLibraryImport and thus should be proper external module:
+                      /** Extension of resolvedFileName. This must match what's at the end of resolvedFileName. */
+                      extension: Extension;

Have module resolution be independend of --allowJs and --jsx flags · Pull Request #11704 · microsoft/TypeScript · GitHub

Uh oh!

Have module resolution be independend of --allowJs and --jsx flags - #11704

Have module resolution be independend of --allowJs and --jsx flags#117048 commits merged intomastermicrosoft/TypeScript:masterfrom refactor_module_resolutionmicrosoft/TypeScript:refactor_module_resolutionCopy head branch name to clipboard

Conversation

Deleted user (ghost) commented Oct 18, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Have module resolution be independend of --allowJs and --jsx flags#11704
8 commits merged into
masterfrom
refactor_module_resolution

Deleted user (ghost) commented Oct 18, 2016 •
edited by ghost
Loading