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

[WIP] Cycle 4 Release #67

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

[WIP] Cycle 4 Release #67

wants to merge 41 commits into from

Conversation

jamesqquick
Copy link
Collaborator

@jamesqquick jamesqquick commented Dec 10, 2024

@migueloller
Copy link
Member

The example code snippet getSiteVersion has a lot going on. Does Mintlify support highlighting certain areas of the code?

developer/guides/deploying/netlify.mdx Outdated Show resolved Hide resolved
developer/app-router/installation.mdx Outdated Show resolved Hide resolved
developer/guides/deploying/netlify.mdx Outdated Show resolved Hide resolved
developer/guides/deploying/netlify.mdx Show resolved Hide resolved
developer/guides/deploying/overview.mdx Outdated Show resolved Hide resolved
snippets/app-router/catch-all.mdx Outdated Show resolved Hide resolved
developer/reference/get-site-version.mdx Show resolved Hide resolved
@jamesqquick
Copy link
Collaborator Author

@migueloller for getSiteVersion snippet, I was thinking about trying something much more minimal to your point about it being busy. It then links to the Installation guide for a more in depth walkthrough. Let me know what you think about that.

CleanShot 2024-12-18 at 12 42 17@2x

@migueloller
Copy link
Member

@migueloller for getSiteVersion snippet, I was thinking about trying something much more minimal to your point about it being busy. It then links to the Installation guide for a more in depth walkthrough. Let me know what you think about that.

CleanShot 2024-12-18 at 12 42 17@2x

I think showing it at the top-level that way might be a bit misleading given that it can only be used on RSCs or Route Handlers. What if we did this?

The example below renders the home page at the default locale. For a more comprehensive example check...

import { Page } from '@makeswift/runtime/next'
import { client } from '@/lib/makeswift'

export default function Page() {
  const snapshot = await client.getPageSnapshot('/', {
    siteVersion: siteVersion(),
  })

  if (snapshot == null) notFound()

  return <Page snapshot={snapshot} />
}

Is that too busy?

Copy link
Member

@migueloller migueloller left a comment

Choose a reason for hiding this comment

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

I started adding suggestions where I found usage of getSiteVersion but there were enough where it made sense to stop and let you take care of it. Also, we'll want to update other instances of that like the URL get-site-version, etc.

snippets/app-router/catch-all.mdx Outdated Show resolved Hide resolved
developer/reference/get-site-version.mdx Show resolved Hide resolved
@migueloller migueloller dismissed their stale review December 19, 2024 15:56

I'm silly. The API is indeed getSiteVersion. I was thinking too much about the cookies and headers APIs from Next.js and what we wanted to name our function longterm that I thought we had named it siteVersion...

@jamesqquick
Copy link
Collaborator Author

@migueloller were you originally thinking it should be siteVersion() instead of getSiteVersion()? So nothing I need to change with those right?

For the code snippet on getSiteVersion, I think what you have is perfect. Great suggestion!

@migueloller
Copy link
Member

@migueloller were you originally thinking it should be siteVersion() instead of getSiteVersion()? So nothing I need to change with those right?

For the code snippet on getSiteVersion, I think what you have is perfect. Great suggestion!

Yeah, the API should probably be siteVersion to match Next.js since they don't do getCookies or getHeaders. It's a breaking change that we won't do right now, though. I think we do it when we ship @makeswift/nextjs if we do have an API like it

@jamesqquick jamesqquick force-pushed the james/cycle-4-release branch from 3e90482 to 7c05747 Compare January 2, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants