GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Have module resolution be independend of --allowJs and --jsx flags · Pull Request #11704 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented Oct 18, 2016 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

…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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Comment thread

src/compiler/types.ts Outdated

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

Mohamed Hegazy (mhegazy) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this should stay optional. otherwise we break other API

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/compiler/types.ts Outdated

		* 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;

Mohamed Hegazy (mhegazy) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/compiler/utilities.ts Outdated

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

Mohamed Hegazy (mhegazy) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/compiler/program.ts Outdated

		* 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 } {

Mohamed Hegazy (mhegazy) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) commented Oct 18, 2016

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

src/compiler/moduleNameResolver.ts

		/* @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;

Vladimir Matveev (vladima) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why removing @internal?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/compiler/moduleNameResolver.ts Outdated


		/** 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 {

Vladimir Matveev (vladima) Oct 18, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

src/compiler/program.ts

		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) {

Mohamed Hegazy (mhegazy) Oct 26, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

Oct 26, 2016

View reviewed changes

Comment thread

src/compiler/types.ts

		/*
		* 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;

Mohamed Hegazy (mhegazy) Oct 26, 2016

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

optional?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) Oct 26, 2016

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) approved these changes

Oct 26, 2016

View reviewed changes

Mohamed Hegazy (mhegazy) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

just a couple of API comments

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Mohamed Hegazy (mhegazy)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Deleted user (ghost) commented Oct 18, 2016 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.