Skip to content

Conversation

erunion
Copy link
Member

@erunion erunion commented May 11, 2021

🧰 What's being changed?

This adds handling for alternate servers and server variables into a new .server() method.

  • Adds support for handling server variables to api.
  • Adds support for adding server variables to generated snippets.
  • Fixes a bug in our handling of paths with path parameters where the path parameters might not always be in the snippet.
  • Cleaning up some tests that used a createOas fixture in favor of using fuller OpenAPI examples from @readme/oas-examples.

🐳 How does it work?

You can either pass in a full server URL into server:

sdk.server('https://example.com/var1/var2')

Or if you instead want to pass in individual server variable params, you can do so as well:

sdk.server({'https://example.com/{pathParam1}/{pathParam2}', {
  pathParam1: 'var1',
  pathParam2: 'var2',
})

By default, snippets generated with httpsnippet-client-api will be generated with the former syntax.

@erunion erunion added the enhancement New feature or request label May 11, 2021
@erunion erunion marked this pull request as ready for review May 12, 2021 22:57
@gkoberger
Copy link
Contributor

Hmmm okay I like this enough... it's clunky, but there's not much we can really do.

I'd love to eventually figure out a way to add shorthand, but I can't think of any way to do that in a deterministic way. (Especially for non-variable servers where, for example, one is a test server and one is real.)

It would be nice if eventually (no need to do this now) we could let people specify a x-readme-default server and if there's no variables then we default to that and don't make people spell it out?

@erunion
Copy link
Member Author

erunion commented May 12, 2021

It would be nice if eventually (no need to do this now) we could let people specify a x-readme-default server and if there's no variables then we default to that and don't make people spell it out?

We kind of handle this already by treating the first server in servers as their default when we run oas.url(): https://github.com/readmeio/oas/blob/main/tooling/index.js#L148-L151

I'd love to eventually figure out a way to add shorthand, but I can't think of any way to do that in a deterministic way. (Especially for non-variable servers where, for example, one is a test server and one is real.)

We could potentially do this as sdk.config({server: 'dev'}) and base it off the description in the Server Variable Object but the problem with that field is that its fulltext Markdown so for one user it might be dev, another it might be:

sdk.config({ server: 'Our local development server that runs on localhost:3000' });

We could also treat the servers by their index in the servers array, but that makes the request really opaque:

/* oas.servers: [
  { url: 'prod' },
  { url: 'dev' }
] */

sdk.config({ server: 1 });
``

@gkoberger
Copy link
Contributor

gkoberger commented May 12, 2021

Oh great!

I don't like the last one (if no other reason, it's possible a swagger creator could switch the order w/o thinking about it, and break tons of APIs!)

@erunion
Copy link
Member Author

erunion commented May 12, 2021

I don't like the last one (if no other reason, it's possible a swagger creator could switch the order w/o thinking about it, and break tons of APIs!)

Agreed

@erunion
Copy link
Member Author

erunion commented May 13, 2021

@gkoberger What about changing this to:

sdk.server('http://api.example.local:3000/api/')

and:

sdk.server('https://example.com/{pathParam1}/{pathParam2}', {
  pathParam1: 'var1',
  pathParam2: 'var2',
})

@Dashron
Copy link

Dashron commented May 13, 2021

Personally I'm always a fan of explicit config methods such as server. It's nicer in IDEs, can lead to clearer docs, and clearer arguments. It can lead to bloat, but server is important enough that it's probably worth it.

Copy link

@Dashron Dashron left a comment

Choose a reason for hiding this comment

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

just one small comment

@erunion erunion merged commit 1dd8a2e into main May 13, 2021
@erunion erunion deleted the feat/support-multiple-servers branch May 13, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants