Implement file system watcher plugin API #3576
Conversation
mmorhun
requested review from
benoitf,
evidolob,
AndrienkoAleksandr and
akurinnoy
| } | ||
|
|
||
| dispose(): void { | ||
| if (this.subscriberData.unsubscribe) { |
There was a problem hiding this comment.
Up to you, but it would be nice dispose onDidCreateEmitter, onDidUpdateEmitter, onDidDeleteEmitter too. Example: https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/connection-status-service.ts#L100
Also you could create dispose collection protected readonly toDispose = new DisposableCollection(); and push all Emitters here, and then toDispose.dispose() utilize each of them.
There was a problem hiding this comment.
Thank you, will fix.
| @@ -28,3 +30,13 @@ export namespace Schemes { | |||
| export const File = 'file'; | |||
| export const Untitled = 'untitled'; | |||
| } | |||
|
|
|||
| export function theiaUritoUriCompponents(uri: URI): UriComponents { | |||
There was a problem hiding this comment.
hello, FYI typo on the function name (two p at Components)
There was a problem hiding this comment.
thank you, I'll rename the function.
| type: FileChangeEventType | ||
| } | ||
|
|
||
| export type FileChangeEventType = 'created' | 'updated' | 'deleted'; |
There was a problem hiding this comment.
should created/updated/deleted be constants as well (to use it in uri-components.ts for example)
There was a problem hiding this comment.
Well, they might be, but it is widely spread practice to have enum types of strings. They are safe (compiler cares of it) and even IDE's tools are dealing well with them (so I still have autocompletion and so on).
| import { FileWatcherSubscriberOptions, FileChangeEventType } from '../api/model'; | ||
| import URI from 'vscode-uri'; | ||
|
|
||
| export class InPluginFileSystemWatcherProxy { |
There was a problem hiding this comment.
Yes, makes sense, will add.
| perSubscriberEventEmitter.fire({ uri, type }); | ||
| } else { | ||
| // shouldn't happen | ||
| // if it happans then a message was lost, unsubscribe to make state consistent |
|
|
||
| constructor( | ||
| subscriberData: SubscriberData, | ||
| ignoreCreateEvents: boolean = false, |
There was a problem hiding this comment.
should we just use private keyword there (so no need to add it few lines before) ?
There was a problem hiding this comment.
I do not like to mix fields declaration types.
But ok, I will change it, here is some special case: the fields is assigned in constructor and never changes after, so it is kind of constants just to complete the specification.
| const fileWatcherSubscriberOptions: FileWatcherSubscriberOptions = { globPattern, ignoreCreateEvents, ignoreChangeEvents, ignoreDeleteEvents }; | ||
| // ids are generated by server side to be able handle several subscribers. | ||
| this.proxy.$registerFileSystemWatcher(fileWatcherSubscriberOptions).then((id: string) => { | ||
| // this is safe, because actual subscription happans on server side and response is |
There was a problem hiding this comment.
| // this is safe, because actual subscription happans on server side and response is | |
| // this is safe, because actual subscription happens on server side and response is |
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
The implementation is done in following way: a plugin subscribes for file system evens and specifies filters if any. In plugin thread there is a proxy which sends request to Theia side to register subscriber (and gets back an subscriber id). On Theia side there is manager which stores subscriber's requirements. When an event happens it will be checked by the manager and if it matches - only then - will sent it back to plugin side with subscriber id, so the proxy redirects it to needed client by id without any processing. This approach helps in reducing traffic between main and plugin side.
Resolves #3163