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

feat: add abort control over Device Flow Handle polling #357

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Conversation

panva
Copy link
Owner

@panva panva commented Apr 22, 2021

Example using AbortController.

const ac = new AbortController()

setTimeout(() => ac.abort(), 10000)

try {
  await handle.poll({ signal: ac.signal })
} catch (err) {
  console.log(err)
}

Example using handle.abort().

setTimeout(() => handle.abort(), 10000)

try {
  await handle.poll({ signal: ac.signal })
} catch (err) {
  console.log(err)
}

resolves #355
closes #356

@panva
Copy link
Owner Author

panva commented Apr 22, 2021

@NotMyself please have a look.

I went with a standard AbortController approach thats making rounds in Node.js core, as well as an instance method to abort for runtimes below Node.js 15 where AbortController was introduced.

The important thing is to make the original promise resolve or reject, it should not be left in a pending state. I went with a rejection when aborted.

I also chose to abort on the next interval since it makes the implementation simpler and the expectation clear.

@NotMyself
Copy link

That is much cleaner than what I was doing. If I am reading the sample correctly I should be doing the following in my extension code. Is that correct?

async (progress, token) => {
    vscode.commands.executeCommand(
      'vscode.open',
      vscode.Uri.parse(handle.verification_uri_complete)
    );
    
    const ac = new AbortController();
    token.onCancellationRequested(() => {
      ac.abort();
    });

    const tokenSet = await handle.poll({ signal: ac.signal });

    await keytar.setPassword(
      SECRET_KEY_SERVICE_NAME,
      'token_set',
      JSON.stringify(tokenSet)
    );

    authStatusEventEmitter.fire(tokenSet);
  }

@panva
Copy link
Owner Author

panva commented Apr 22, 2021

Yes, I think so. And it will reject handle.poll() with the next interval check. Are you okay with me proceeding with this PR then?

@NotMyself
Copy link

Yeah, this works for me. Thank you for such a wonderful library. So easy to use.

@panva panva merged commit f6faa68 into main Apr 22, 2021
@panva panva deleted the df-abort branch April 22, 2021 15:21
@panva
Copy link
Owner Author

panva commented Apr 22, 2021

Yeah, this works for me. Thank you for such a wonderful library. So easy to use.

Please consider supporting the library if it provides value to you or your company and this was of help to you. Supporting the library means, amongst other things, that such support and new features will be available in the future.

@NotMyself
Copy link

@panva I work at Auth0 and we do sponsor you. 😁

@panva
Copy link
Owner Author

panva commented Apr 22, 2021

@panva I work at Auth0 and we do sponsor you. 😁

standard reply ;)

@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2021
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.

2 participants