Language service proxy by RyanCavanaugh · Pull Request #12231 · microsoft/TypeScript (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Implements basic language service proxy plugins

steve8708, WanderWang, MarkPieszak, niieani, AndreaCardamone, stephenlaughton, guncha, MartinHelmut, hheexx, SlurpTheo, and 3 more reacted with thumbs up emoji DanielRosenwasser, steve8708, WanderWang, MarkPieszak, and hheexx reacted with hooray emoji steve8708, mulyoved, MarkPieszak, and hheexx reacted with heart emoji

return this.cachedUnresolvedImportsPerFile;
}

public static resolveModule(moduleName: string, initialDir: string, host: ServerHost, log: (message: string) => void): {} {

Choose a reason for hiding this comment

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

Why not just reuse the module resolver the TS compiler uses elsewhere? I understand that the module resolver is a lot more complicated thanks to its exposure the the compiler options, but it should be configurable to load just JS, right? At least, it'd probably be better not to duplicate the folder traversal/lookup logic in two places?

Choose a reason for hiding this comment

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

I considered this but didn't think it was worthwhile given that we'd have to mock out a CompilerOptions guaranteed to behave the same as NodeJS's resolution (which still differs from our own given how we crack open package.jsons and other stuff). I'm actually going to switch this to just use require directly anyway now that I've learned how to set up a different initial search path.

Choose a reason for hiding this comment

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

@RyanCavanaugh Shouldn't require at least be abstracted behind a call to sys?

Choose a reason for hiding this comment

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

@weswigham I don't really see a point - is anyone plausibly going to write a plugin that works under a host that's not node?

Choose a reason for hiding this comment

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

what about substituting require in unit tests?

Choose a reason for hiding this comment

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

And I'm sure someone like @basart would like to support angular and other plugins in his cloud IDE.

Choose a reason for hiding this comment

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

🌹

Choose a reason for hiding this comment

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

I don't really see a point - is anyone plausibly going to write a plugin that works under a host that's not node?

@RyanCavanaugh Isn't there an intent for this to work in Visual Studio? Our Chakra host would need to implement its own require logic in that case.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

It's already been refactored into sys as suggested.

return;
}

if (typeof require === "undefined") {

Choose a reason for hiding this comment

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

Rather than using the global require directly, shouldn't this be abstracted as a host detail under sys?

const items: string[] = [];
for (const plugin of this.plugins) {
if (plugin.getExternalFiles) {
items.push(...plugin.getExternalFiles(this));

Choose a reason for hiding this comment

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

If I'm not mistaken, this calls plugin code which can throw - any errors should probably be handled and logged?

Choose a reason for hiding this comment

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

Yup. we need to wrapp this in a try..catch block to make sure the project is not in an invalid state.

const proxy = Object.create(null);
const langSvc: any = info.languageService;
for (const k of Object.keys(langSvc)) {
proxy[k] = function () {

Choose a reason for hiding this comment

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

The LS codebase (and the API's consumers) are not resilient to exceptions being thrown from these API methods, however consumers of the plugin API are not bound by any contract forbidding exceptions. Is there some way we can force a safer construction of these proxy methods whereby execution errors caused by improper plugins are appropriately logged and recorded, rather than surfaced as ICEs?

}
}

export interface PluginCreateInfo {

Choose a reason for hiding this comment

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

I'm not sure if this is still a priority, but should plugins be given the opportunity to be well-behaved with respect to a cancellation token?

Choose a reason for hiding this comment

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

+1, plugins should be monitoring cancellation requests similar to normal LS methods


// Note: 'require' needs the path to be all forward slashes and absolute
initialDir = normalizeSlashes(nodeSystem.resolvePath(initialDir));
mod.paths.unshift(initialDir);

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Definitely aware (see earlier comment in this function for a link to an SO question about this). I considered this to be preferable to re-implementing their module resolution algorithm since we don't necessarily know how it works in all cases. Thoughts?

export interface CommandLineOptionOfListType extends CommandLineOptionBase {
type: "list";
element: CommandLineOptionOfCustomType \| CommandLineOptionOfPrimitiveType;
element: CommandLineOptionOfCustomType \| CommandLineOptionOfPrimitiveType	TsConfigOnlyOption;

Choose a reason for hiding this comment

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

This reads weird and needs a comment if deliberate. By definition, I would assume a "TsConfigOnlyOption" can't be provided as a CommandLineOption?

Choose a reason for hiding this comment

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

It's how everything here is named. Note that CommandLineOptionBase has an isTSConfigOnly property, which you would think would have to be false by definition

}

for (const pluginConfigEntry of options.plugins) {
const searchPath = combinePaths(this.configFileName, "..");

Choose a reason for hiding this comment

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

This reads like it's going up a folder to the parent of the folder with the config file, but I assume the intent is just to remove the tsconfig.json and leave the folder. Looking at the implementation of combinePaths, I believe it's just going to result in these being concatenated with a directory separator (e.g. /src/project/tsconfig.json/..), which may work for 'resolveModule', but seems a little bizarre.

Choose a reason for hiding this comment

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

Will change to path.dirname

Choose a reason for hiding this comment

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

getDirectoryPath rather

}
catch (e) {
this.projectService.logger.info(`Plugin activation failed: ${e}`);
this.projectService.logger.info(`Plugin object was: ${pluginModule}`);

Choose a reason for hiding this comment

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

Isn't this just going to log as [object]? Did you mean to JSON.stringify it?

Choose a reason for hiding this comment

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

If it's an object, yes. In this case I needed to find out that it wasn't null, wasn't undefined, wasn't a class, wasn't a primitive, and wasn't a function (the angular module was returning a factory function when I expected an object in this case). I would be worried calling JSON.stringify here as it could trigger a secondary exception during a property getter

Choose a reason for hiding this comment

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

You might want to browse the spec (http://es5.github.io/#x15.12.3) and run some testing to verify, but JSON.stringify never throws (even for undefined, null, NaN, etc..) and usually outputs what you would want.

Choose a reason for hiding this comment

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

It does throw.

If stack contains value then throw a TypeError exception because the structure is cyclical.

Choose a reason for hiding this comment

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

Right, I should have been clearer. I meant based on the property type (i.e. addressing the comment I needed to find out that it wasn't null, wasn't undefined, wasn't a class, wasn't a primitive, and wasn't a function).

Choose a reason for hiding this comment

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

Updated to do some better logging of Objects. I believe Object.keys cannot throw on an object input

////

goTo.marker();
verify.quickInfoIs('Proxied x: number[]Check');

Choose a reason for hiding this comment

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

The tests seem a little light for a new feature. We need to test what happens when the plugin fails to load (either throws on initialization or can't be found), if the plugin config is misconfigured, if the proxied API returns invalid results, etc... We need to ensure we're either resilient or give an actionable error to help identify the cause, else we'll be troubleshooting a lot of issues that aren't ours 😟

Hate to be bumping, but is there anything that still prevents this from being ready?

How does plugins work under watch mode?

For example if I create a plugin to require resx file for resources, I would like to automatically watch all languages besides the imported file and generate a typescript equivalent code.
This also means if some stops importing the resx file I need to stop watching.

public static resolveModule(moduleName: string, initialDir: string, host: ServerHost, log: (message: string) => void): {} {
const resolvedPath = normalizeSlashes(host.resolvePath(combinePaths(initialDir, "node_modules")));
log(`Loading moduleNamefrom{moduleName} from moduleNamefrom{initialDir} (resolved to ${resolvedPath})`);
const result = host.require(resolvedPath, moduleName);

Choose a reason for hiding this comment

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

i would wrap this in a try/catch block and log the exception

Choose a reason for hiding this comment

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

or just do this in enableProxy

Choose a reason for hiding this comment

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

require can't throw. It returns a module or an error.

Choose a reason for hiding this comment

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

can you please put it as a comment to ServerHost.require since we don't have any other ways to declare/enforce this contract?

languageServiceHost: this.lsHost,
serverHost: this.projectService.host
};
if (pluginModule.create === undefined && typeof pluginModule === "function") {

Choose a reason for hiding this comment

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

is there a reason why we have two ways to define a module? why would not create take ts as well? how would it get ts?

Choose a reason for hiding this comment

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

Can we define the shape of a plugin in an interface as well.

Choose a reason for hiding this comment

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

This is how angular's module loading scheme works. We could force everyone to follow this pattern, or allow both ways. Thoughts?

getExternalFiles(): string[] {
const items: string[] = [];
for (const plugin of this.plugins) {
if (plugin.getExternalFiles) {

Choose a reason for hiding this comment

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

typeof plugin.getExternalFiles !== "function"

const items: string[] = [];
for (const plugin of this.plugins) {
if (plugin.getExternalFiles) {
items.push(...plugin.getExternalFiles(this));

Choose a reason for hiding this comment

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

Yup. we need to wrapp this in a try..catch block to make sure the project is not in an invalid state.

const diagnostics = [...sanityCheckProgram.getSyntacticDiagnostics(), ...sanityCheckProgram.getSemanticDiagnostics(), ...sanityCheckProgram.getGlobalDiagnostics()];
if (diagnostics.length) {
const flattenedDiagnostics = diagnostics.map(d => ts.flattenDiagnosticMessageText(d.messageText, "\n")).join("\n");
const flattenedDiagnostics = diagnostics.map(d => ts.flattenDiagnosticMessageText(d.messageText, "\n") + ' at ' + d.file.fileName + ' line ' + d.start).join("\n");

Choose a reason for hiding this comment

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

nit:

`${ts.flattenDiagnosticMessageText(d.messageText, "\n")} at d.file.fileNameline{d.file.fileName} line d.file.fileNameline{d.start}

setTimeout,
clearTimeout
clearTimeout,
require(initialDir: string, moduleName: string) {

Choose a reason for hiding this comment

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

can we move require from sys to ServerHost?

it looks like currently only tsserver will use it
if we do it in tsserver we can use our module resolution implementation and then do require with resolved path without relying on undocumented properties

clearTimeout?(timeoutId: any): void;
}

export type RequireResult = { module: {}, error: undefined } \| { module: undefined, error: {} };

Choose a reason for hiding this comment

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

I would move this to types.ts or server.ts

Looks good to me. @vladima any final comments?

}

export function nodeModuleNameResolver(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, cache?: ModuleResolutionCache): ResolvedModuleWithFailedLookupLocations {
export function nodeModuleNameResolver(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, cache?: ModuleResolutionCache, jsOnly = false): ResolvedModuleWithFailedLookupLocations {

Choose a reason for hiding this comment

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

nit: can we instead of using default parameter split this function into two:

export function nodeModuleNameResolver(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): ResolvedModuleWithFailedLookupLocations { return nodeModuleNameResolverWorker(moduleName, containingFile, compilerOptions, host, /cache/ undefined, /jsOnly/ false); } /* @internal */ export function nodeModuleNameResolverWorker(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, cache: ModuleResolutionCache, jsOnly): ResolvedModuleWithFailedLookupLocations { ... }

and make internal callers use nodeModuleNameResolverWorker instead of nodeModuleNameResolver
Reason: I think it is preferable to keep public interface of the compiler clean and not pollute it with internal intricacies

Choose a reason for hiding this comment

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

👍

},
setTimeout,
clearTimeout
clearTimeout,

Choose a reason for hiding this comment

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

nit: extra comma

Choose a reason for hiding this comment

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

👍


public languageServiceEnabled = true;

protected lsHost: LSHost;

Choose a reason for hiding this comment

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

can you also make it readonly? AFAIR nobody should change it once LSHost is created

Choose a reason for hiding this comment

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

👍

serverHost: this.projectService.host
};

if (typeof pluginModuleFactory !== "function") {

Choose a reason for hiding this comment

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

move the check before making info

Choose a reason for hiding this comment

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

👍

Choose a reason for hiding this comment

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

LGTM, with a nit about making plugin aware of cancellation token

Is this what's described in #11976 ?

I can see that @angelozerr says in #13436 (comment) thanks @mhegazy for your answer. Now I see how to plugins to load are declared (in the tsconfig.json).

But #13436 doesn't actually include any answer with such an example. Was the usage described elsewhere? Offline?

Design notes mention this PR only in passing too.

It looks like a huge layer of functionality implemented on the sly! 😎 ㊙️

We still need to add documentation for this feature. This is scheduled for TS 2.3 at the moment.

orta mentioned this pull request

Feb 24, 2017

With these changes in place, can we already achieve the holy grail of extensions and introduce arbitrary types in the system or is that still some way off? There are two use-cases that I'm thinking of:

const foo = Relay.QLfragment on Foo { name, bar { count } } require.ensure("lib/bar", (error, bar) => bar.baz())

In the first case, the rest of the code should be type checked as if foo had a type { value: {name: string, bar: { count: number }}} and the second is a way to code-split with webpack and the bar in the callback should be typed as the module from "lib/bar".

The type checker in checker.ts is very encapsulated so there's no real way to extend any of the methods. Some of this could be accomplished by changing the SourceFile right after it's parsed, but that seems a little hacky.

@guncha, this is just an LS extension, it does not change any of the types in checking phase. i believe you are looking for a different extension point, something like #3136

This was referenced

May 22, 2017

microsoft locked and limited conversation to collaborators