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

Add Support for strategy Prop from Next.js Script Library #150

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

Unsleeping
Copy link

This pull request enhances the next-runtime-env library by adding support for the strategy prop from the Next.js next/script component. The change addresses issues #145 and #147, and helps improve runtime environment configuration retrieval on the not-found page.

Key Changes:

  • Introduced the strategy prop to allow customization of script loading behavior (e.g., beforeInteractive, afterInteractive).
  • Default strategy is set to beforeInteractive, following current practices. This ensures backward compatibility, but the default strategy can now be adjusted as needed.

Observations:

During testing, I noticed that with the beforeInteractive strategy, I was unable to retrieve the runtime environment configuration on the not-found page.
When using afterInteractive, the environment config is successfully retrieved and assigned to window on the not-found page.

So, I need to know about the exact limitations of the beforeInteractive strategy, as it seems hardcoded to load scripts early. I might have misunderstood something, and further clarification would be helpful.

For reference, here's the relevant Next.js documentation on the beforeInteractive strategy: Next.js Script beforeInteractive Documentation.

@Unsleeping
Copy link
Author

@HofmannZ Would love for you to take a look at this

@Unsleeping Unsleeping deleted the branch expatfile:development September 26, 2024 21:54
@Unsleeping Unsleeping closed this Sep 26, 2024
@Unsleeping Unsleeping deleted the development branch September 26, 2024 21:54
@Unsleeping Unsleeping restored the development branch September 27, 2024 06:05
@Unsleeping Unsleeping reopened this Sep 27, 2024
This reverts commit 24c3844.
Copy link
Member

@HofmannZ HofmannZ left a comment

Choose a reason for hiding this comment

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

@Unsleeping - I'm happy to add some flexibility to the package, just curious how safe afterInteractive is to use. We chose beforeInteractive because it runs before any Next.js code and before any page hydration occurs, allowing us to use the env vars everywhere.

afterInteractive loads early but after some hydration on the page occurs.

CC: @davevanhoorn

@HofmannZ
Copy link
Member

Following up on my prior message, the real problem is this bug in Next.js: vercel/next.js#69567 (comment)

@Unsleeping
Copy link
Author

@HofmannZ

The issue was discussed in a thread, and here’s a brief summary:

When rendering the not-found page, Next.js throws a specific error and reacts to all errors in the same way — it stops rendering the page, including the root layout where next-runtime-env configuration assignment occurs. As explained in the thread, the problem is that Next.js halts the rendering of any page when an error occurs, whether it’s a NotFoundError or another error. As a result, the server stops rendering the page, including all layout elements. It’s important to note that the initial HTML returned by the server does not contain any specific layout elements.

Basically, the rendering of a page is stopped whenever any error is encountered. Regardless of whether the error is caused by notFound() or a regular new Error(), the route rendering stops, and the HTML returned by the server lacks the necessary tags for scripts using the beforeInteractive strategy. When hydration finishes, the script is added to the HTML, but by that time, the main app-bootstrap script has already executed, and its impact is lost. Therefore, the beforeInteractive loading strategy does not work on pages with errors.

However, all other loading strategies, such as afterInteractive or lazyOnLoad, work correctly.

I’m actively testing the afterInteractive strategy right now and haven’t encountered any bugs yet. They may surface soon, but there are bugs #145 and #147 related to similar behavior.

That’s why I kept beforeInteractive as the default but allowed the option to change it per project if necessary.

We need to look into this more deeply and possibly switch from beforeInteractive to afterInteractive, instead of providing the option to choose the strategy prop for the script.

@Unsleeping
Copy link
Author

Unsleeping commented Oct 16, 2024

@HofmannZ

The only issue I've encountered so far after switching to afterInteractive is when I use runtime environment variables to set constants that are later used throughout the application. There are cases where, on the client side, the constant is initialized before the environment configuration is populated into the global window object.

For example, something like this:

export const apiUrl = env("NEXT_PUBLIC_API_URL");

In this case, window.__ENV might not be available yet, causing the constant to be set with an empty value. On the server side, this isn’t an issue since env(key) just returns process.env[key]. But on the client side, it’s recommended to retrieve environment variables at the point of use, using a getter function to ensure the values are available when needed.

In the Next.js documentation for the beforeInteractive prop, it clearly states:

Load the script before any Next.js code and before any page hydration occurs.

This explains why the previous behavior wasn't breaking, but after switching to afterInteractive, it caused issues in my case.

p.s. same with lazyOnload strategy

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