ory / sdk

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

feat(ts): use native fetch instead of axios #256

Closed DASPRiD closed 5 months ago

DASPRiD commented 1 year ago

With Node 18 having native support for the fetch API, there is no need anymore to have a separate library for network communication.

This PR switches the typescript-axios generator out against the typescript-fetch one. If backward compatibility for older node versions is still wanted, this could instead be released as a separate package (@ory/client-fetch).

This would hugely benefit frontend projects, as users would have to download one dependency less, thus reducing loading times.

BREAKING CHANGES: This patch requires a fetch polyfill on NodeJS versions 16 and lower.

Checklist

aeneasr commented 1 year ago

Until Node 14 and 16 are gone, I don't think we can replace this in the existing client :/ We also need to check if the generated code is different from the axios generator.

DASPRiD commented 1 year ago

So what would be an acceptable way to make this an available flavor today? Maybe have it as a third-party package?

DASPRiD commented 1 year ago

FYI: For the moment I created a namespaces package I'm using successfully. https://www.npmjs.com/package/@dasprid/ory-client-fetch

The initialization of the Configuration is slightly different, as now one has to pass credentials: 'include'.

DASPRiD commented 1 year ago

Good news for Node < 18 on that package: You can inject a custom fetchApi in the options, so e.g. node-fetch similar.

DASPRiD commented 1 year ago

Here's a small summary of what I noticed so far:

Using it with native fetch

This config is required when using it with native fetch, either NodeJS 18+ or browser:

export const frontendApi = new FrontendApi(new Configuration({
    basePath: 'http://kratos-url',
    credentials: 'include',
    headers: {
        'Accept': 'application/json',
    },
}));

Using it with fetch ponyfill

This is only required on NodeJS < 18. One can either use node-fetch or undici:

import fetch from 'node-fetch';

export const frontendApi = new FrontendApi(new Configuration({
    basePath: 'http://kratos-url',
    credentials: 'include',
    headers: {
        'Accept': 'application/json',
    },
    fetchApi: fetch,
}));

Other notable changes

Successful responses

The generated API has s light difference in usage. Specifically a successful response from any of the API calls will just yield a promise with decoded body in it , instead of an axios object with a data attribute.

Error responses

Also errors will be of type ResponseError and contain the response object under the response key.

Dates

Dates and date-times are not left alone as strings anymore but converted to JavaScript Date objects. From what I can see not a big issue with @ory/elements, as it never uses those fields, so there should be nothing breaking there.

The conversion can be turned off, but not in openapi-generator 5.4.0, this requires the latest stable version 6.

Minor typing changes

On version 5.4.0, the typings are otherwise identical. When switching to version 6, the typings do differ semantically, but that's not a major issue. I only noticed it because the typings of my package started to collide with the typings of @ory/client used by @ory/elements. This would be a non-issue when @ory/client would use the newer generator.

Conclusion

Overall this shows that publishing two different libraries for JavaScript is a bad idea, as @ory/elements relies on the @ory/client typings.

That being said, it should be safer at the moment to do that transition than doing it later when more and more people have adopted @ory/client.

My current workaround

After some playing around, I figured out a way to make @ory/elements work with my package. The trick is to use a not so well known feature of npm to alias one package to another:

npm i @ory/client@npm:@dasprid/ory-client-fetch
aeneasr commented 1 year ago

Thank you so much for the write-up! Unfortunately it sounds like there are some serious breaking changes so we would need to properly plan a migration to this library. We could consider publishing an additional SDK to npm which uses the fetch library though. If we move to fetch we should first ensure if that generator is the one the community will work on in the future primarily.

DASPRiD commented 1 year ago

Sounds good to me. What would be the next steps to take here; Anything I can help with?

DASPRiD commented 1 year ago

By the way, interestingly enough your documentation already mentions the possibility that each new SDK version can break backwards compatibility:

https://www.ory.sh/docs/sdk#sdk-backwards-compatibility

aeneasr commented 1 year ago

By the way, interestingly enough your documentation already mentions the possibility that each new SDK version can break backwards compatibility:

https://www.ory.sh/docs/sdk#sdk-backwards-compatibility

Yes, but we still want to keep the major breaking changes small :) To keep users sane.

I think there's nothing you can do from your side at the moment. The workaround looks like a good interim solution. We will probably add @ory/client-next and at some point rename @ory/client to @ory/client-legacy and @ory/client-next to @ory/client. WDYT @jonas-jonas

DASPRiD commented 1 year ago

That sounds like a good approach to me. And with npm aliasing, this will then not interfere with with your supporting libraries like @ory/elements and @ory/integration. You can then advise your users to do this:

npm install @ory/client@npm:@ory/client-next
jonas-jonas commented 1 year ago

We will probably add @ory/client-next and at some point rename @ory/client to @ory/client-legacy and @ory/client-next to @ory/client.

Sounds good to me. The changes to the actual SDK methods seem small to me, so having a -next flavor of the client and giving ample warnings to users, before renaming would be a good solution.

There is also https://openapi-generator.tech/docs/generators/typescript/ which allows defining pluggable/custom HTTP clients. It's still WIP and experimental, but it seems that it's setup to replace the existing typescript generators, IIUC. For Ory clients, it might be an option, to release that and let users use their own implementations, or some pre-made ones from NPM (I guess OpenAPI will release default ones for the common fetching libs).

DASPRiD commented 1 year ago

After further testing I noticed an issue with the schema specs, I reported a the bug here: https://github.com/ory/sdk/issues/260

TartanLeGrand commented 1 year ago

:up:

Sacramentix commented 1 year ago

I've tested to make a version of the sdk that work on deno with fetch. Here is the repo https://github.com/Sacramentix/ory-sdk-fetch/tree/master/clients/client/typescript-deno . I've deploy it on denoland so we can easily test it https://deno.land/x/sacramentix_ory_client@v.1.1.39 .

However there is few difference between axios and fetch generator that lead to few method not working.

The sdk work, but i 've encountered a problem with the method FrontendApi.toSession which do the request '/sessions/whoami' under the hood. It take 3 parameters 'xSessionToken?: string, cookie?: string, _options?: Configuration' When we want to use it with cookie we pass undefined as the xSessionToken. But generated code then proceed to add a header xSessionToken with a string value of undefined. Therefore it fails as the xSessionToken is invalid even through there is a valid cookie.

TartanLeGrand commented 1 year ago

Hello :wave: @Sacramentix I don't understand what do you whant, but if I can help you for some tests or review tell me :smile:

xqqp commented 11 months ago

With node 14 and node 16 having reached end-of-life and node 18 being supported by kratos (https://github.com/ory/kratos/pull/3492), what prevents this from being merged?

MaximeCheramy commented 7 months ago

The version of axios currently used contains known vulnerabilities: https://github.com/advisories/GHSA-wf5p-g6vw-rhxx

Could you please consider using fetch, or at least update axios?

Thank you.

MaximeCheramy commented 7 months ago

@DASPRiD your commit 6e2cde3ff765ef2aaec1a7beb98c2f8ede471b4c is not needed anymore. But that would be great if we could merge this and drop support of node 14 and 16.

Jorgagu commented 5 months ago

@aeneasr @vinckr any update on this topic ?

aeneasr commented 5 months ago

https://github.com/ory/sdk/pull/341

MaximeCheramy commented 5 months ago

Awesome news thanks. I currently use @ory/hydra-client, is there a @ory/hydra-client-fetch too?

aeneasr commented 5 months ago

Upon the next hydra release