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

Confused about what is optional in terms of Infra #348

Open
jxdp opened this issue Jan 10, 2024 · 3 comments
Open

Confused about what is optional in terms of Infra #348

jxdp opened this issue Jan 10, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation

Comments

@jxdp
Copy link

jxdp commented Jan 10, 2024

https://open-next.js.org/advanced/architecture

Having some difficulty understanding what is required and what is optional, and which options/infra apply to which features.

For example, the diagram makes it seem like the Cache (bucket + files) is optional, but the description makes it sound very important. Is it just an ISR feature or is it always needed? Unless there is a good reason not to, I think the docs could do a better job of making it clearer which features each piece of infra is required by.

The options page is also somewhat confusing in a similar vain. For example, https://open-next.js.org/advanced/options#experimental-disable-dynamodb-cache says it is "unsafe" to disable dynamo, but as as far as I can tell it should be safe unless the optional ISR feature is being used. I don't think it makes sense to say it is "unsafe" to disable an optional feature? Again, I think this can be easily improved by adding a label to say which features it is relevant to. This could be as simple as changing the headings, eg Experimental disable dynamodb cache (ISR)

I've been using OpenNext since v0 and I think it's fantastic. I used the original docs to build some stacks that we deploy in several applications at work, and I couldn't have done that without the original v0 docs. I have been trying to make some updates to those stacks and am finding the new docs much less detailed and imagine it would be significantly more difficult for somebody new to the project to do what I was able to do quite easily back then. Some of this is surely just a result of adding more features--which is great--but ease of adoption is very a important aspect of this project (unless I'm mistaken) and that means making the docs detailed enough for somebody to write their own IaC.

@conico974
Copy link
Collaborator

You're right, docs could always be better. I'll try to explain things more clearly.

There is 2 different cache used in open-next:

  • The S3 cache, which is used for both ISR AND SSG (anything with getStaticProps or every non SSR app router pages, and even SSR when Partial Prerendering will go out of beta)
  • The DynamoDb cache that is used for next/cache based revalidation. revalidatePath and revalidateTag only, so it could be safely ignored for page router.

Both of those cache could be ignored, but we chose to put it under a dangerous flag because it breaks some functionality that may not seem obvious in the first place for everyone.

revalidatePath for example, is just a wrapper around revalidateTag, but most people assume it works the same way as res.revalidate in page router do.
Likewise, most people don't realise that SSG pages are treated by next internally almost exactly as ISR pages (due to on demand revalidation).

Disabling the S3 cache and using SSG, doesn't even break your app if you do not have fallback: false, but it could lead to some very inconsistent state.
For example if your page request some data, it will request data everytime it is not a HIT in the CDN, but the JSON/RSC and the html are two different entry in your CDN (even more than 2 for RSC), and they might be requested at different time resulting in potentially different data for the same page.

For the IAC part, i totally agree, and that's why for V3 rc we include a reference CDK implementation that should help people to more easily create their own IAC.

If you could create a PR to improve the docs, it would be greatly appreciated

@conico974 conico974 added the documentation Improvements or additions to documentation label Jan 10, 2024
@jxdp
Copy link
Author

jxdp commented Jan 13, 2024

@conico974 I'm so pleased that there is going to be a reference implementation--I think that will probably answer the questions that sometimes docs struggle to.

To be clear, I don't have a problem with calling something dangerous per se, it's more that it isn't clear why it is dangerous.

Happy to submit some changes, but I'll need to clarify a few things further in order to do so.

Here are my thoughts based on your explanation above:

  • the S3 Cache should be considered part of "Core" instead of part of "ISR" (as per the diagram) since it also effects SSG and soon SSR.
    • S3 buckets are very simple from an IaC perspective and cheap in terms of cost, so I can't see any reason not to do this.
  • the docs should make it clear what applies to app router only or pages router only where appropriate.
    • the (vast?) majority of NextJS apps/teams that would benefit from moving to OpenNext (ie probably a few years old, need to leverage other cloud infra, maxed out Vercel team size or have a large bill, etc) are using the pages router and will want to write IaC to support their pages router application without having to become an expert in app router
  • I really like the diagram but perhaps it is has the potential to mislead (as per the prior Cache discussion)

Looking forward to your thoughts--what would you suggest to improve the docs?

@conico974
Copy link
Collaborator

Sorry for the delay,

the S3 Cache should be considered part of "Core" instead of part of "ISR" (as per the diagram) since it also effects SSG and soon SSR.

Not sure that we should put it in Core, renaming the ISR part to ISR/SSG would probably be a better idea.
As for the SSR part, it will only be if you specifically enable Partial Prerendering (only for app router, and optional), and to be clear this is not fully supported at the moment. (The cache are not properly populated, and the roundtrip to S3 might make it totally worthless, especially if the static part is fast to compute)

We should probably add a specific page about partial prerendering, because it cannot work the same way that it does on vercel. On vercel the page is served from the CDN, very likely from a cloudflare worker, that's impossible to do outside of vercel at the moment, you have to reach the server function. To make it work we would have to move the cache outside of the server function in the routing/middleware part (something that V3 will make possible, maybe in V3.1) and reconstruct the streamed html from the precomputed rendered part and the postponed RSC

the docs should make it clear what applies to app router only or pages router only where appropriate

We should probably add a paragraph about that in the docs, in the architecture page.
At the moment there is only 1 difference from an IAC point of view, dynamoDB that is not necessary for pages router. Of course this might change in the future. We should probably also add that streaming is useless for page router and ISR/SSG (even with app router)

I really like the diagram but perhaps it is has the potential to mislead (as per the prior Cache discussion)

We should probably keep the diagram, we might need to revisit it a little bit and add a disclaimer that you should read the rest of the docs to really understand everything.
I think the diagram should be for people that just want to have a broad picture of what's going on under the hood, if you want to write your own IAC, you should read everything else

Anyway, the docs will need a big revamp with the V3 release with the introduction of the open-next.config.ts file and all the part that can be overriden. For example the S3 cache become the Incremental Cache and the DynamoDB cache become the Tag cache

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

No branches or pull requests

2 participants