search

ory / sdk

The place where ORY's SDKs are being auto-generated
Apache License 2.0
141 stars 85 forks source link

refactor: deno compatibility #203

Closed arnemolland closed 2 years ago

arnemolland commented 2 years ago

Related Issue or Design Document

ory/kratos#2600

Checklist

Further comments

While Axios is a widely used HTTP client, it's based on XHR and is not exported as an ES Module, which results in tools and runtimes such as Deno not being able to use the SDK.

The promise-based Fetch API is stable and openapi-generator's typescript-fetch generator is stable and has feature parity with the typescript-axios generator. In order to cover all runtimes, the generated client will need to include either a Node-compatible fetch global, e.g. isomorphic-fetch or a polyfill.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

aeneasr commented 2 years ago

Unfortunately typescript-fetch doesn't work in many scenarios and is also a big breaking change; so unfortunately we can't accept this change :(

aeneasr commented 2 years ago

We could introduce a second SDK thougah, e.g. client-js-fetch that uses the fetch API

arnemolland commented 2 years ago

We could introduce a second SDK thougah, e.g. client-js-fetch that uses the fetch API

Sounds good! I'll update the PR to reflect that approach.

aeneasr commented 2 years ago

Thanks! For transparency, I'll have to check with someone though first. It's quite confusing when there are several SDKs available to choose from and we had and still have problems with that at the moment. So I can't guarantee that it will be merged.

aeneasr commented 2 years ago

By the way, is the problem with deno that it uses XHR, or that it's not an es module?

aeneasr commented 2 years ago

fyi @kmherrmann

arnemolland commented 2 years ago

By the way, is the problem with deno that it uses XHR, or that it's not an es module?

Both, but CDNs like skypack auto-converts most npm packages, so the only issue for the Ory SDK is XHR as Deno doesn't implement it

dmitrigrabov commented 2 years ago

Thanks! For transparency, I'll have to check with someone though first. It's quite confusing when there are several SDKs available to choose from and we had and still have problems with that at the moment. So I can't guarantee that it will be merged.

Thank you for looking at this @aeneasr. Now that fetch is natively supported by Node, I suspect many will switch to it instead of using an external dependency on Axios for api calls. One downside of Axios based apis is that they will throw rather than return for non 200 responses, which creates an inconsistency in behaviour that is often a source of confusion.

I appreciate the difference in behaviour would create overhead for you in terms of documentation and support. Is there any other way to create a typescript-fetch that would give us access to the functionality without creating more work for you?

aeneasr commented 2 years ago

Thank you for looking at this @aeneasr. Now that fetch is natively supported by Node, I suspect many will switch to it instead of using an external dependency on Axios for api calls.

That makes sense! The problem with the node ecosystem though is that many providers (including vercel, netlify, ...) will stay on node 14/16 for a long time. For those systems, XHR is the correct approach as it doesn't require polyfills which in some runtimes don't work.

I appreciate the difference in behaviour would create overhead for you in terms of documentation and support. Is there any other way to create a typescript-fetch that would give us access to the functionality without creating more work for you?

The biggest problem is confusion - we already have quite a lot of SDK versions out there (kratos, keto, hydra, oathkeeper, meta) and people don't know which ones to use. By adding yet another variant we are just going to increase this problem.

I think deno is a really cool project, and I would love to see Ory's SDKs compatible with it. Just need to find a good way to achieve that.

Do you maybe have a repo I can take a look at to understand what the problem is with deno?

arnemolland commented 2 years ago

Do you maybe have a repo I can take a look at to understand what the problem is with deno?

I've got a simple repro here. What I'm seeing is that Skypack is able to convert Axios to ESM but it stumbles when trying to do a request.

Logs

```bash TypeError: adapter is not a function at dispatchRequest2 (https://cdn.skypack.dev/-/axios@v0.21.4-66TDYaHQZf9OXMAPUc8F/dist=es2019,mode=imports/optimized/axios.js:817:10) at Axios.request (https://cdn.skypack.dev/-/axios@v0.21.4-66TDYaHQZf9OXMAPUc8F/dist=es2019,mode=imports/optimized/axios.js:1138:15) at Function.wrap [as request] (https://cdn.skypack.dev/-/axios@v0.21.4-66TDYaHQZf9OXMAPUc8F/dist=es2019,mode=imports/optimized/axios.js:7:15) at https://cdn.skypack.dev/-/@ory/client@v0.0.1-alpha.189-8kvxxSwUSup52hWThGR9/dist=es2019,mode=imports/optimized/@ory/client.js:140:21 at https://cdn.skypack.dev/-/@ory/client@v0.0.1-alpha.189-8kvxxSwUSup52hWThGR9/dist=es2019,mode=imports/optimized/@ory/client.js:2288:116 at async Server.#respond (https://deno.land/std@0.149.0/http/server.ts:298:18) ```

I'm trying to do some reading on the subject in order to understand and pinpoint the problem a bit better, Deno has some resources on Node/NPM package interop here.

arnemolland commented 2 years ago

I also think that having a single JS/TS SDK is the way to go, I got a bit confused at all the different ones initially as well. It should be achievable (ref. Deno's node/npm package interop) to make non-breaking changes that allows for Deno compatibility. I'm playing around with different generators and XHR polyfills to see if I get something working.

arnemolland commented 2 years ago

I see that the plain typescript generator outputs a fetch-based client that also implements isomorphic-fetch which should in theory solve any compat issues for Node < 18. Works fine with Node 12/14/16 and Chrome/Safari but I've yet to test with react-native.

The only thing that throws me off is that the generator falsely identifies all the details fields of the client spec (v0.1.0-alpha.12) as non-object types, and throws on the fact that they have additionalProperties set to true. Removing additionalProperties from all details fields fixes it (by the spec it's true by default), but that's an issue for the openapi-generator project. Only happens with the typescript generator though, not the typescript-axios nor typescript-fetch.

Ref. OpenAPITools/openapi-generator/issues/12973

arnemolland commented 2 years ago

I'm pretty certain that additionalProperties are true by default, even though it's an openapi-generator issue that it throws when explicitly set, would it be okay to leave it out where explicitly set to true and let it be implicit in order for the typescript generator to work?

cc @aeneasr

arnemolland commented 2 years ago

Sorry for flooding the PR, but I've implemented a small demo of a Deno-compatible SDK here.

In order for it to work, I had to:

  1. Leave out all "additionalProperties": true ref the bug above.
  2. Update generation script to use the typescript generator.
  3. Update the implementation to use the new code generated.

Notes:

aeneasr commented 2 years ago

Looksl ike Deno is working on backwards compatibility :)

https://deno.com/blog/changes

arnemolland commented 2 years ago

Looksl ike Deno is working on backwards compatibility :)

https://deno.com/blog/changes

Hoping for the best! If the Node compatibility layer patches out XHR then we're all good to go. At least it removes to need for third-party CDNs, which is neat.

arnemolland commented 2 years ago

Closing as Deno support approach is depending on the upcoming Node compatibility layer either supporting or not supporting XHR. If not, Node 18 with Fetch API implementation is soon hitting LTS, and Fetch polyfills can be used with older Node versions.