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

apiv2 #2762

Merged
merged 16 commits into from
Nov 4, 2020
Merged

apiv2 #2762

merged 16 commits into from
Nov 4, 2020

Conversation

bkendall
Copy link
Contributor

@bkendall bkendall commented Oct 29, 2020

Description

Creating a new apiv2 package that's in typescript and will allow us to change out the current api package (based on request). This is built using node-fetch which is fairly light-weight.

The exported Client module has a generic request method for... generic requesting, but also has 'verb' methods matching various HTTP verbs to make most requests very simple to write.

Scenarios Tested

I've kept this to the hosting:channel:* commands and have been using them as my tests. Seems to be working quite well!

  • hosting:channel:list works
  • hosting:channel:create works (and with --expires)
  • hosting:channel:deploy works
  • hosting:channel:delete works

@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Oct 29, 2020
src/apiv2.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mbleigh mbleigh left a comment

Choose a reason for hiding this comment

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

I think creating a parallel package is a great idea (allows us to migrate gradually without worrying about getting it 100% right all at once). But if we're gonna do that, feels like we can kinda just go nuts on the new implementation and make the interface exactly what we want without worrying about backwards compat. For instance, maybe we should take an instantiated client approach, something like:

import { Client, hostingOrigin } from "../apiv2";

const api = new Client({
  origin: hostingOrigin,
  apiVersion: "v1beta1",
  auth: true
});

const channel = await api.post<CreateChannelRequest, Channel>({
  ttl: `${ttlMillis / 1000}s`
});

that's just an example, but we could try to build a more opinionated client experience and maybe pull in some OP-specific utilities (for instance, imagine a listPaginated() method that auto-paginates using OP parlance).

I'm not opposed to a "the existing thing, but TypeScript", but it looks like this approach is changing a fair amount of the surface but not really as a ground-up reimagining.

Another example here is that we no longer really need to vary requests by scope -- that's not a thing we do anymore since we rely on IAM to be our permissions gate.

src/apiv2.ts Outdated Show resolved Hide resolved
src/apiv2.ts Outdated Show resolved Hide resolved
src/apiv2.ts Outdated Show resolved Hide resolved
src/apiv2.ts Outdated Show resolved Hide resolved
src/apiv2.ts Outdated Show resolved Hide resolved
@bkendall bkendall marked this pull request as ready for review November 3, 2020 23:30
this.opts.auth = true;
}
if (this.opts.urlPrefix.endsWith("/")) {
this.opts.urlPrefix = this.opts.urlPrefix.substring(0, this.opts.urlPrefix.length - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't substr preferred now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I recall. They're two different functions (which can be basically substituted here): the second argument for substr is the number is the length to return, the second argument to substring is the index to stop at. Using 0 for the first argument, either is technically fine.

src/apiv2.ts Show resolved Hide resolved
src/apiv2.ts Show resolved Hide resolved
Copy link
Contributor

@mbleigh mbleigh left a comment

Choose a reason for hiding this comment

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

Please manually test codepaths for all swapped implementations before merging.

src/apiv2.ts Outdated Show resolved Hide resolved
src/apiv2.ts Outdated Show resolved Hide resolved
@bkendall bkendall merged commit 9d90a94 into master Nov 4, 2020
@bkendall bkendall deleted the bk-ts-api2 branch November 4, 2020 19:34
@bkendall bkendall added the cleanup: request PRs for removing the request module from the CLI label Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA. cleanup: request PRs for removing the request module from the CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants