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

feat: add nextjs example and supporting libraries #240

Closed
wants to merge 40 commits into from

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Oct 17, 2024

Closes ory/integrations#61

BREAKING CHANGES: This patch changes the options structure for useSesssion.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Sorry, but this triggered me during the presentation 😉

examples/nextjs-app-router/next/types.ts Outdated Show resolved Hide resolved
examples/nextjs-app-router/next/types.ts Outdated Show resolved Hide resolved
examples/nextjs-app-router/next/types.ts Outdated Show resolved Hide resolved
examples/nextjs-app-router/next/types.ts Outdated Show resolved Hide resolved
examples/nextjs-app-router/next/types.ts Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member Author

aeneasr commented Oct 18, 2024

🤣

@aeneasr

This comment was marked as outdated.

@aeneasr aeneasr closed this Oct 18, 2024
@aeneasr aeneasr reopened this Oct 18, 2024
@aeneasr
Copy link
Member Author

aeneasr commented Oct 18, 2024

I'm stuck trying to create the @ory/nextjs library and importing them in the two examples. Kinda lost here, would appreciate some help @jonas-jonas

examples/nextjs-app-router/nextjs/index.ts Outdated Show resolved Hide resolved
packages/nextjs/package.json Show resolved Hide resolved
packages/nextjs/src/app/index.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
import { OryCardContent } from "@ory/elements-react"
Copy link
Member Author

Choose a reason for hiding this comment

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

This should actually be `OryCardLayout

Suggested change
import { OryCardContent } from "@ory/elements-react"
import { OryCardLayout } from "@ory/elements-react"

Copy link
Member

Choose a reason for hiding this comment

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

If we do export that file, it should be

This should actually be

Suggested change
import { OryCardContent } from "@ory/elements-react"
import { OryCardLayout } from "@ory/elements-react/theme"

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonas-jonas it's now:

// app/auth/layout.tsx

// Copyright © 2024 Ory Corp
// SPDX-License-Identifier: Apache-2.0

import { DefaultCardLayout } from "@ory/elements-react/theme"

export default DefaultCardLayout


export function DefaultCardLayout({ children }: PropsWithChildren) {
return (
<main className="p-4 pb-8 flex items-center justify-center flex-col gap-8 min-h-screen">
Copy link
Member Author

Choose a reason for hiding this comment

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

The problem kinda is that we have the card element, but no backdrop element, so the card just is on the top left of the screen. We should add a layout component or background component. Not sure where to put it, so i put it here. But it's kinda weird because we don't override it anywhere, we just import it (see nextjs app router example app)

Copy link
Member

Choose a reason for hiding this comment

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

This is intentional. IMO, if you use Ory Elements, you should need to decide where to put the elements on the page, so it's up to the consumer to implement this. I guess it doesn't hurt to export this, but it's a really optional element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed and this is how it's done now.

@jonas-jonas

This comment was marked as outdated.

@jonas-jonas jonas-jonas force-pushed the aeneasr/add-nextjs-app-router-example-2 branch 2 times, most recently from 079ab44 to 8c3bc44 Compare October 30, 2024 17:08
@jonas-jonas jonas-jonas force-pushed the aeneasr/add-nextjs-app-router-example-2 branch from 8c3bc44 to 7f3319b Compare November 8, 2024 10:44
@aeneasr aeneasr marked this pull request as ready for review November 11, 2024 11:29

export function DefaultCardLayout({ children }: PropsWithChildren) {
return (
<main className="p-4 pb-8 flex items-center justify-center flex-col gap-8 min-h-screen">
Copy link
Member

Choose a reason for hiding this comment

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

This is intentional. IMO, if you use Ory Elements, you should need to decide where to put the elements on the page, so it's up to the consumer to implement this. I guess it doesn't hurt to export this, but it's a really optional element.

Comment on lines 41 to 44
export function useSearchParams() {
const router = useRouter()
return toSearchParams(router.query)
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this exported in the package? If so, this will lead to it showing up in code completion if you have this package installed, which is not great, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only

// Copyright © 2024 Ory Corp
// SPDX-License-Identifier: Apache-2.0
"use client"
export { useRegistrationFlow } from "./registration"
export { oryPageRouterConfig } from "./config"

is exported in utils/index.js

@@ -0,0 +1,3 @@
import { OryCardContent } from "@ory/elements-react"
Copy link
Member

Choose a reason for hiding this comment

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

If we do export that file, it should be

This should actually be

Suggested change
import { OryCardContent } from "@ory/elements-react"
import { OryCardLayout } from "@ory/elements-react/theme"

packages/nextjs/src/app/actions.ts Outdated Show resolved Hide resolved
@aeneasr aeneasr self-assigned this Nov 14, 2024
Makefile Outdated
Comment on lines 13 to 23
build:
npm run build

dev:
npm run build

Copy link
Member

Choose a reason for hiding this comment

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

revert?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted the scripts to now use the nx commands, which I always had to lookup in the GitHub ci config.

@@ -0,0 +1,49 @@
This is a [Next.js](https://nextjs.org) project bootstrapped with
Copy link
Member

Choose a reason for hiding this comment

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

This needs a proper README.

body {
color: var(--foreground);
background: var(--background);
font-family: Arial, Helvetica, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

We use Inter in the designs, so the examples should, too.

Copy link
Member

Choose a reason for hiding this comment

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

what is this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is included in the default example. We should probably add an api route that verifies the cookie here?

examples/nextjs-pages-router/pages/fonts/GeistMonoVF.woff Outdated Show resolved Hide resolved
})

xit("should use window.location.origin if VERCEL_URL is not provided", () => {
// Not sure if this works
Copy link
Member

Choose a reason for hiding this comment

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

What do we need to check to make sure this works?

Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow be able to modify window.location.origin which I have not been able to get working in this test

import { OryConfig } from "../types"
import { isProduction } from "./sdk"

export function enhanceConfig(
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
export function enhanceConfig(
export function enhanceOryConfig(

And it needs docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

test("understands public suffix list", () => {
expect(
guessCookieDomain(
"https://spark-public.s3.amazonaws.com/dataanalysis/loansData.csv",
Copy link
Member

Choose a reason for hiding this comment

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

Lol, where does this come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

An LLM, maybe.

new URL(selfUrl).toString().replace(/\/$/, ""),
)
}
export function rewriteJsonResponse<T extends object>(
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs tests. Seems like a pretty complicated function, maybe some docs/explanations would be good?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have several tests for this in describe("rewriteJsonResponse", () => {

I added docs

@@ -9,6 +9,7 @@
"skipLibCheck": true,
"baseUrl": ".",
"paths": {
"@ory/nextjs": ["packages/nextjs/src/index.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is needed with npm workspaces, why did you add it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@aeneasr aeneasr force-pushed the aeneasr/add-nextjs-app-router-example-2 branch from 53178c6 to 254f36e Compare November 19, 2024 10:26
@aeneasr aeneasr force-pushed the aeneasr/add-nextjs-app-router-example-2 branch from 254f36e to 4311b66 Compare December 5, 2024 10:27
@aeneasr aeneasr requested a review from jonas-jonas December 5, 2024 10:28
@aeneasr aeneasr closed this Dec 5, 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.

Support Next.js App Router
3 participants