Skip to content
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

Language service proxy #12231

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Language service proxy #12231

merged 2 commits into from
Feb 14, 2017

Conversation

RyanCavanaugh
Copy link
Member

Implements basic language service proxy plugins

@RyanCavanaugh
Copy link
Member Author

cc @chuckjaz

@@ -142,6 +154,35 @@ namespace ts.server {
return this.cachedUnresolvedImportsPerFile;
}

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

@weswigham weswigham Nov 15, 2016

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

🌹

Copy link
Member

@DanielRosenwasser DanielRosenwasser Dec 29, 2016

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DanielRosenwasser VS17 uses tsserver + node so it won't be a problem

Copy link
Member Author

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") {
Copy link
Member

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));
Copy link
Member

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?

Copy link
Contributor

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 () {
Copy link
Member

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?

@@ -90,6 +90,18 @@ namespace ts.server {
}
}

export interface PluginCreateInfo {
Copy link
Member

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?

Copy link
Contributor

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

@RyanCavanaugh
Copy link
Member Author

@mhegazy ping

@RyanCavanaugh
Copy link
Member Author

Angular-side PR is up at angular/angular#13716

@RyanCavanaugh
Copy link
Member Author

Ping anyone...

@RyanCavanaugh
Copy link
Member Author

@mhegazy @vladima ?

@RyanCavanaugh
Copy link
Member Author

Please?


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

Choose a reason for hiding this comment

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

I'd be wary of this. 'module.paths' isn't documented at all on the Node.js site (https://nodejs.org/dist/latest-v6.x/docs/api/modules.html). I'd assume this is an internal property which could change or disappear at any time.

Copy link
Member Author

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?

@@ -3358,7 +3364,7 @@ namespace ts {
/* @internal */
export interface CommandLineOptionOfListType extends CommandLineOptionBase {
type: "list";
element: CommandLineOptionOfCustomType | CommandLineOptionOfPrimitiveType;
element: CommandLineOptionOfCustomType | CommandLineOptionOfPrimitiveType | TsConfigOnlyOption;
Copy link
Member

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?

Copy link
Member Author

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, "..");
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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}`);
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Contributor

@gcnew gcnew Jan 19, 2017

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.

Copy link
Member

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

Copy link
Member Author

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');
Copy link
Member

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 😟

@skreborn
Copy link

skreborn commented Feb 8, 2017

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

@prabirshrestha
Copy link
Member

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 ${moduleName} from ${initialDir} (resolved to ${resolvedPath})`);
const result = host.require(resolvedPath, moduleName);
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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") {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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) {
Copy link
Contributor

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));
Copy link
Contributor

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.

@@ -167,7 +167,7 @@ function generateProtocolFile(protocolTs: string, typeScriptServicesDts: string)
const sanityCheckProgram = getProgramWithProtocolText(protocolDts, /*includeTypeScriptServices*/ false);
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");
Copy link
Contributor

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.fileName} line ${d.start}

@@ -569,7 +573,31 @@ namespace ts {
}
},
setTimeout,
clearTimeout
clearTimeout,
require(initialDir: string, moduleName: string) {
Copy link
Contributor

@vladima vladima Feb 9, 2017

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

@@ -45,6 +45,8 @@ namespace ts {
clearTimeout?(timeoutId: any): void;
}

export type RequireResult = { module: {}, error: undefined } | { module: undefined, error: {} };
Copy link
Contributor

@mhegazy mhegazy Feb 14, 2017

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

@RyanCavanaugh
Copy link
Member Author

@mhegazy any other comments?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 14, 2017

Looks good to me. @vladima any final comments?

@@ -683,14 +683,14 @@ namespace ts {
}
}

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 {
Copy link
Contributor

@vladima vladima Feb 14, 2017

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -582,7 +582,7 @@ namespace ts {
}
},
setTimeout,
clearTimeout
clearTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra comma

Copy link
Member Author

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;
Copy link
Contributor

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

Copy link
Member Author

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") {
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@vladima vladima left a comment

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

@RyanCavanaugh RyanCavanaugh merged commit 87b780d into microsoft:master Feb 14, 2017
@DanielRosenwasser
Copy link
Member

🎉

@mihailik
Copy link
Contributor

mihailik commented Feb 15, 2017

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! 😎 ㊙️

@RyanCavanaugh RyanCavanaugh deleted the ls-proxy branch February 15, 2017 18:38
@mhegazy
Copy link
Contributor

mhegazy commented Feb 15, 2017

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

@guncha
Copy link

guncha commented Apr 21, 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.QL`fragment 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.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 21, 2017

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.