-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Language service proxy #12231
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,19 +91,38 @@ namespace ts.server { | |
} | ||
} | ||
|
||
export interface PluginCreateInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, plugins should be monitoring cancellation requests similar to normal LS methods |
||
project: Project; | ||
languageService: LanguageService; | ||
languageServiceHost: LanguageServiceHost; | ||
serverHost: ServerHost; | ||
config: any; | ||
} | ||
|
||
export interface PluginModule { | ||
create(createInfo: PluginCreateInfo): LanguageService; | ||
getExternalFiles?(proj: Project): string[]; | ||
} | ||
|
||
export interface PluginModuleFactory { | ||
(mod: { typescript: typeof ts }): PluginModule; | ||
} | ||
|
||
export abstract class Project { | ||
private rootFiles: ScriptInfo[] = []; | ||
private rootFilesMap: FileMap<ScriptInfo> = createFileMap<ScriptInfo>(); | ||
private lsHost: LSHost; | ||
private program: ts.Program; | ||
|
||
private cachedUnresolvedImportsPerFile = new UnresolvedImportsMap(); | ||
private lastCachedUnresolvedImportsList: SortedReadonlyArray<string>; | ||
|
||
private readonly languageService: LanguageService; | ||
// wrapper over the real language service that will suppress all semantic operations | ||
protected languageService: LanguageService; | ||
|
||
public languageServiceEnabled = true; | ||
|
||
protected readonly lsHost: LSHost; | ||
|
||
builder: Builder; | ||
/** | ||
* Set of files names that were updated since the last call to getChangesSinceVersion. | ||
|
@@ -150,6 +169,17 @@ namespace ts.server { | |
return this.cachedUnresolvedImportsPerFile; | ||
} | ||
|
||
public static resolveModule(moduleName: string, initialDir: string, host: ServerHost, log: (message: string) => void): {} { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RyanCavanaugh Shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about substituting There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🌹 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@RyanCavanaugh Isn't there an intent for this to work in Visual Studio? Our Chakra host would need to implement its own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DanielRosenwasser VS17 uses tsserver + node so it won't be a problem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already been refactored into |
||
const resolvedPath = normalizeSlashes(host.resolvePath(combinePaths(initialDir, "node_modules"))); | ||
log(`Loading ${moduleName} from ${initialDir} (resolved to ${resolvedPath})`); | ||
const result = host.require(resolvedPath, moduleName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or just do this in enableProxy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you please put it as a comment to |
||
if (result.error) { | ||
log(`Failed to load module: ${JSON.stringify(result.error)}`); | ||
return undefined; | ||
} | ||
return result.module; | ||
} | ||
|
||
constructor( | ||
private readonly projectName: string, | ||
readonly projectKind: ProjectKind, | ||
|
@@ -237,6 +267,10 @@ namespace ts.server { | |
abstract getProjectRootPath(): string | undefined; | ||
abstract getTypeAcquisition(): TypeAcquisition; | ||
|
||
getExternalFiles(): string[] { | ||
return []; | ||
} | ||
|
||
getSourceFile(path: Path) { | ||
if (!this.program) { | ||
return undefined; | ||
|
@@ -804,10 +838,12 @@ namespace ts.server { | |
private typeRootsWatchers: FileWatcher[]; | ||
readonly canonicalConfigFilePath: NormalizedPath; | ||
|
||
private plugins: PluginModule[] = []; | ||
|
||
/** Used for configured projects which may have multiple open roots */ | ||
openRefCount = 0; | ||
|
||
constructor(configFileName: NormalizedPath, | ||
constructor(private configFileName: NormalizedPath, | ||
projectService: ProjectService, | ||
documentRegistry: ts.DocumentRegistry, | ||
hasExplicitListOfFiles: boolean, | ||
|
@@ -817,12 +853,64 @@ namespace ts.server { | |
public compileOnSaveEnabled: boolean) { | ||
super(configFileName, ProjectKind.Configured, projectService, documentRegistry, hasExplicitListOfFiles, languageServiceEnabled, compilerOptions, compileOnSaveEnabled); | ||
this.canonicalConfigFilePath = asNormalizedPath(projectService.toCanonicalFileName(configFileName)); | ||
this.enablePlugins(); | ||
} | ||
|
||
getConfigFilePath() { | ||
return this.getProjectName(); | ||
} | ||
|
||
enablePlugins() { | ||
const host = this.projectService.host; | ||
const options = this.getCompilerOptions(); | ||
const log = (message: string) => { | ||
this.projectService.logger.info(message); | ||
}; | ||
|
||
if (!(options.plugins && options.plugins.length)) { | ||
this.projectService.logger.info("No plugins exist"); | ||
// No plugins | ||
return; | ||
} | ||
|
||
if (!host.require) { | ||
this.projectService.logger.info("Plugins were requested but not running in environment that supports 'require'. Nothing will be loaded"); | ||
return; | ||
} | ||
|
||
for (const pluginConfigEntry of options.plugins) { | ||
const searchPath = getDirectoryPath(this.configFileName); | ||
const resolvedModule = <PluginModuleFactory>Project.resolveModule(pluginConfigEntry.name, searchPath, host, log); | ||
if (resolvedModule) { | ||
this.enableProxy(resolvedModule, pluginConfigEntry); | ||
} | ||
} | ||
} | ||
|
||
private enableProxy(pluginModuleFactory: PluginModuleFactory, configEntry: PluginImport) { | ||
try { | ||
if (typeof pluginModuleFactory !== "function") { | ||
this.projectService.logger.info(`Skipped loading plugin ${configEntry.name} because it did expose a proper factory function`); | ||
return; | ||
} | ||
|
||
const info: PluginCreateInfo = { | ||
config: configEntry, | ||
project: this, | ||
languageService: this.languageService, | ||
languageServiceHost: this.lsHost, | ||
serverHost: this.projectService.host | ||
}; | ||
|
||
const pluginModule = pluginModuleFactory({ typescript: ts }); | ||
this.languageService = pluginModule.create(info); | ||
this.plugins.push(pluginModule); | ||
} | ||
catch (e) { | ||
this.projectService.logger.info(`Plugin activation failed: ${e}`); | ||
} | ||
} | ||
|
||
getProjectRootPath() { | ||
return getDirectoryPath(this.getConfigFilePath()); | ||
} | ||
|
@@ -839,6 +927,21 @@ namespace ts.server { | |
return this.typeAcquisition; | ||
} | ||
|
||
getExternalFiles(): string[] { | ||
const items: string[] = []; | ||
for (const plugin of this.plugins) { | ||
if (typeof plugin.getExternalFiles === "function") { | ||
try { | ||
items.push(...plugin.getExternalFiles(this)); | ||
} | ||
catch (e) { | ||
this.projectService.logger.info(`A plugin threw an exception in getExternalFiles: ${e}`); | ||
} | ||
} | ||
} | ||
return items; | ||
} | ||
|
||
watchConfigFile(callback: (project: ConfiguredProject) => void) { | ||
this.projectFileWatcher = this.projectService.host.watchFile(this.getConfigFilePath(), _ => callback(this)); | ||
} | ||
|
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 reads weird and needs a comment if deliberate. By definition, I would assume a "TsConfigOnlyOption" can't be provided as a CommandLineOption?
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.
It's how everything here is named. Note that
CommandLineOptionBase
has anisTSConfigOnly
property, which you would think would have to be false by definition