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

refactor: flatten project config and make arguments optional #236

Closed
wants to merge 1 commit into from

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Oct 15, 2024

BREAKING CHANGES: The configuration OryClientConfiguration has changed. Everything previously under the project key is now at the root level and optional:

 {
   sdk: { ... },
-  project: {
-    registration_enabled: true,
-    ...
-  }
   registration_enabled: true,
   ...
 }

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

BREAKING CHANGES: The configuration `OryClientConfiguration` has changed. Everything previously under the `project` key is now at the root level and optional:

```patch
 {
   sdk: { ... },
-  project: {
-    registration_enabled: true,
-    ...
-  }
   registration_enabled: true,
   ...
 }
```
@aeneasr aeneasr self-assigned this Oct 15, 2024
@aeneasr aeneasr requested a review from jonas-jonas October 15, 2024 08:29
@@ -89,7 +89,7 @@ export function OryTwoStepCard() {
{step === ProcessStep.ChooseAuthMethod && (
<>
{flowType === FlowType.Login && (
<BackButton href={config.project.login_ui_url} />
<BackButton href={config.login_ui_url ?? '/login'} />
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should make assumptions about the library setup here. If we do, this will cause unexpected behavior for customers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm ok, but it kinda sucks that I have to define all of these values when I want to use the SDK

*
* Defaults to true.
*/
registration_enabled?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

These should all be required, because otherwise we have another layer of defaults in the Ory Network stack.

One possible improvement is, to make it possible for the library to "auto discover" these values from the project's API endpoints, so that you don't need to pass them in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I see. But then the user has to keep those in sync? So I assume we need an endpoint for this?

Copy link
Member

Choose a reason for hiding this comment

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

We already have an endpoint for it, but it's internal only for now.

We can expose it, though, and that was the plan. Just need to make sure it doesn't expose anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I think we should solve this for the release, otherwise it's really cumbersome to set up. And then it should be removed from the config in my view as we would just use the SDK URL to fetch the data.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think we should solve this for the release, otherwise it's really cumbersome to set up.

But it's just a one time setup, where we can provide a copy & pasteable snippet. I don't think it's that critical.

And we should still provide the option to override all of these settings manually, for local dev or feature flags.

@aeneasr aeneasr marked this pull request as draft October 15, 2024 10:54
@aeneasr aeneasr closed this Oct 15, 2024
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.

2 participants