Add support for middleware in onProgress

[DanTup]

@dbaeumer I think this is working as expected now, however I have some type errors I can't figure out how to resolve:

[dbaeumer]

The type is only syntactic sugar that helps the TS compiler with type inference. Since middleware.handleProgress uses a type argument the compiler doesn't know what to do since neither handleProgress nor the type parameter is typed. You can type the type param as type: typeof WorkDoneProgress.type which should help the compiler with type inference.

[DanTup]

@dbaeumer tried that, but still didn't work. I tried adding explicit types to all of them, but it still fails (seems like it might not like a handler for only a specific set of types for <P>?). The error is:

Type '(type: lsclient.ProgressType<lsclient.WorkDoneProgressBegin | lsclient.WorkDoneProgressReport | lsclient.WorkDoneProgressEnd>, token: lsclient.ProgressToken, params: lsclient.WorkDoneProgressBegin | lsclient.WorkDoneProgressReport | lsclient.WorkDoneProgressEnd, next: lsclient.HandleProgressSignature<lsclient.WorkDo...' is not assignable to type '<P>(this: void, type: ProgressType<P>, token: ProgressToken, params: P, next: HandleProgressSignature<P>) => void'.
  Types of parameters 'type' and 'type' are incompatible.
    Type 'ProgressType<P>' is not assignable to type 'ProgressType<WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd>'.
      Type 'P' is not assignable to type 'WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd'.
        Type 'P' is not assignable to type 'WorkDoneProgressEnd'.

I tried many combinations of types in the args, but can't seem to make it pass. I've added as any for now to make it compile and ensured the tests pass.

I don't think there's anything else outstanding (besides removing the as any), so lmk if you have any other feedback. Thanks!

(also rebased on latest master).

[dbaeumer]

@DanTup I checked-out our code and I don't see any compile errors. Neither in the watch, compile task or the TS server.

[dbaeumer]

I see (as any)

[dbaeumer]

The problem seems to be that type inference doesn't work with properties (which in on or the other way makes sense). So all I can think about for now that in the middleware we make this more explicit. (we don't have a general request middleware either).

So I opt we add instead the following method:

	handleWorkDoneProgress?: (this: void, type: typeof WorkDoneProgress.type, token: ProgressToken, params: WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd, next: HandleProgressSignature<WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd>) => void;

When VS Code will support partial result progress we will add another method.

[dbaeumer]

And then in the test

			middleware.handleWorkDoneProgress = (type, token, params, next) => {
				if (token === progressToken) {
					middlewareEvents.push(params);
					if (params.kind === 'end')
					{setImmediate(resolve);}
				}
				return next(type, token, params);
			};

[DanTup]

@dbaeumer Yeah, I added the as any to silence them to ensure everything worked. I'm not sure how to remove as any without causing errors - though I think that's the only issue (with the as any, the tests pass and I think it's all worked as expected).

I'll try your suggestions shortly, thanks!

btw - I haven't yet tested this "for real" in my project (only the test here), though I would like to - but I couldn't figure out how to correctly build the package in a way I can use it from my extension.. Is there an easy way to do that?

[dbaeumer]

And even better. Now that the method is explict you can remove the type param since it is not necessary anymore

[dbaeumer]

And we should rename HandleProgressSignature to HandleWorkDoneProgressSignature and remove the type param

[DanTup]

@dbaeumer then what should BaseLanguageClient.onProgress look like? It currently takes type: ProgressType<P> so wouldn't we need to do if (type == WorkDoneProgress) { to decide whether to call the middleware?

[dbaeumer]

Yes, that makes sense.

[DanTup]

@dbaeumer ok, assuming I understood correctly, I have a remaining type error here:

I believe this is because my if check mentioned above (line 2820) isn't good enough for P to be narrowed to WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd. Is there a better way to handle that condition that would to this?

[DanTup]

Actually, it seems like if (type == WorkDoneProgress) isn't correct - it's never true. I thought maybe it should be if (type == WorkDoneProgress.type) however TS reports those types never overlap, so I'm unsure how to check that.

This condition will always return 'false' since the types 'ProgressType<P>' and 'ProgressType<WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd>' have no overlap.

I've pushed the changes so far (which have types errors) if you want to have a poke - I'm not sure where to go next. Thanks!

[dbaeumer]

@DanTup the if needs to be if (type === WorkDoneProgress.type). And I think we need a hard cast for the params. May be we can make a is function. I will have a look.

[DanTup]

the if needs to be if (type === WorkDoneProgress.type)

That's what seemed right to me, though I got the message above.

I will have a look.

Thanks! Let me know if you want me to do any more :-)

[dbaeumer]

OK. Here you go: Change the WorkDoneProgress namespace to (observe the new is function):

export namespace WorkDoneProgress {
	export const type = new ProgressType<WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd>();

	export function is(value: ProgressType<any>): value is typeof type {
		return value === type;
	}
}

However the compiler still complains of calling something that is types as P with WorkDoneProgressBegin | WorkDoneProgressReport | WorkDoneProgressEnd since there is no overlap. Since we know better I would do:

handleWorkDoneProgress(token, params, () => handler(params as unknown as P));

See the cast to unknown before casting to P

That makes it work for me.

[dbaeumer]

And I for change the if (type === WorkDoneProgress.type) to if (WorkDoneProgress.is(type)) {

[DanTup]

@dbaeumer looks like that did it, thanks! I've pushed the changes.

Is there an easy way for me to build/reference this package in my local extension to verify it does everything I need without needing it to be published? I've run npm run rebuildAll, should symlinking the node_modules/vscode-language* folder sin my package to the respective lib folders here do the right thing?

Thanks!

[DanTup]

Looks like at some point I committed some formatting changes - I think they're probably for the better (I usually have format-on-save enabled), but if you'd prefer they weren't in this PR let me know and I'll undo the changes that aren't related to this!

[dbaeumer]

The format changes look ok to me.

[DanTup]

Great! Did you see the other question in the previous comment about using the package locally? The folder structure looked slightly different to what's in my existing node_modules, so I wasn't sure if that's the right way. It'd come in handy for testing my other PR too. Thanks! :-)

[dbaeumer]

Regarding

btw - I haven't yet tested this "for real" in my project (only the test here), though I would like to - but I couldn't figure out how to correctly build the package in a way I can use it from my extension.. Is there an easy way to do that?

Couple of options:

• you can package it locally and then install it into where you need it. npm pack
• you can try to use npm link. But I have to say I am not an expert with this and whenever I tried it it failed for me :-)

[dbaeumer]

Let me know when you think the PR is ready to be merged.

[DanTup]

@dbaeumer I couldn't get npm link to work either - I think it was confused by the multiple package names. Using npm pack and then npm install <path-to-tarball> seems to have worked though.

I have one issue when running it in my real project/server, and that is that only the first progress seems to fire the middleware. However, I'm unsure whether this is my server behaving incorrectly. I'm creating the token once, and then sending begin/end and then another begin/end. Is this valid? Should I be able to reuse the same token even after it "ended", or am I supposed to re-create it.

If I'm supposed to be able to reuse it, something seems wrong (though I'm not sure where) and I'll try to do some digging later.

Thanks!

[dbaeumer]

Actually a token obtained by a create shouldn't be used after an end anymore. As you can see here the progress part is removed after a end has been received.

[dbaeumer]

I clarified this in the spec.

[DanTup]

@dbaeumer aha, that explains it. Changing the server to re-create the tokens seems to fix it - my middleware is firing exaactly as expected:

So if you're happy with this, it's good to merge. Thanks!

[DanTup]

Actually, I just triggered this error. Just trying to track it down. In the LSP log it seems like I always did create->begin->end, so this is probably related to my changes.

[Error - 4:00:14 PM] Registering progress handler for token ANALYZING failed.
Error: Progress handler for token ANALYZING already registered
    at Object.onProgress (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:20095:23)
    at LanguageClient.onProgress (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:4657:45)
    at new ProgressPart (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:12788:41)
    at createHandler (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:25905:13)
    at handleRequest (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:19624:41)
    at processMessageQueue (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:19505:17)
    at Immediate._onImmediate (/Users/danny/.vscode/extensions/dart-code.dart-code-3.17.0-dev/out/dist/extension.js:19495:13)
    at processImmediate (internal/timers.js:439:21)

[DanTup]

@dbaeumer I found another issue in the server that's making it difficult for me to debug the above, so might not get to the bottom of this today. Please hold off merging this, and I'll ping you when I've figured out what's happening (I think it's a bug here, though it's possible it's a server timing issue - my server tests are failing in strange ways too).

[DanTup]

@dbaeumer I tracked down the issue in my server - I was reusing a token ID for the events, but because it was async, I wasn't correctly waiting for the old ones end event to be sent before trying to reuse it. I've fixed that, and the error has gone away (it occurred even without this PR before). I'll make a note to add some debouncing though, as we're sending way more of these events than we need to (eg. after each document modification).

I have pushed a slight change to the tests here though, so the servers sample progress now creates a token. This also meant removing the (non-middleware) onProgress handler (because the library registers its own when the token is created). Let me know if you think this is ok or we should revise it.

Otherwise, this appears to be all working correctly for me and it's good to go.

[dbaeumer]

Thanks a lot for providing the PR. Highly appreciated.