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

Bump to @ory/client 1.0.1 #225

Closed
wants to merge 3 commits into from
Closed

Bump to @ory/client 1.0.1 #225

wants to merge 3 commits into from

Conversation

drev74
Copy link

@drev74 drev74 commented Dec 2, 2022

Related Issue or Design Document

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added the necessary documentation within the code base (if appropriate).

Further comments

@drev74
Copy link
Author

drev74 commented Dec 2, 2022

@vinckr 👀

@jonasbadstuebner
Copy link
Contributor

Could we update this to bump to v1.1.0 right away?

Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. A few things need to be updated. And I think you can update to v1.1, AFAIK there were no changes to the frontend.

return {
apiBaseUrl: apiBaseUrl,
kratosBrowserUrl: apiBaseUrl,
sdk,
frontend: new FrontendApi(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
frontend: new FrontendApi(),
...sdk

That way you have frontend (and the other APIs) available, but they're setup with the correct configuration.

import { Application, NextFunction, Request, Response } from "express"

export interface RouteOptions {
sdk: V0alpha2ApiInterface
frontend: FrontendApi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
frontend: FrontendApi
frontend: FrontendApi
identity: IdentityApi
oauth2: OAuth2Api

await sdk
.createSelfServiceLogoutFlowUrlForBrowsers(req.header("cookie"))
await frontend
.createBrowserLogoutFlow(req.header("cookie"))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing these parameters as named parameters, they're now passed in a single params object. Could you update the usage of these methods as well? It won't compile otherwise.

jonasbadstuebner added a commit to jonasbadstuebner/ory-selfservice-ui-node that referenced this pull request Dec 12, 2022
@jonasbadstuebner
Copy link
Contributor

@jonas-jonas Please have a look at #228
I followed the requested changes from here, bumped to v1.1.0 and also implemented an optional Hydra consent page.

Benehiko pushed a commit that referenced this pull request Dec 14, 2022
Benehiko added a commit that referenced this pull request Dec 14, 2022
* linting: refactoring as in #225

* chore: cleanup

* chore: bump ory/integrations

* style: format

* chore: cleanup

* fix: login and registration hydra integration

Co-authored-by: Jonas Badstübner <[email protected]>
@Benehiko
Copy link
Contributor

Hi @drev74

Thank you that you took the time to work on this, unfortunately we require this repository on the latest sdk and are a bit pressed for time according to our internal roadmap and merged #229

Please do continue to contribute to the Ory community - we really do appreciate it ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants