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

Use next-runtime-env for dynamic envs #515

Merged
merged 2 commits into from
Dec 2, 2024
Merged

Conversation

MikeShi42
Copy link
Contributor

Disabling ASO had some issues with app navigation clashing with query parameter updates from all our nuqs usage. Switching to next-runtime-env and also changing the preset environment variables to NEXT_PUBLIC_HDX_LOCAL_DEFAULT_CONNECTIONS and NEXT_PUBLIC_HDX_LOCAL_DEFAULT_SOURCES, which is required for the package, but also makes it nice and clear that these are going to be publicly exposed environment variables.

Copy link

changeset-bot bot commented Nov 30, 2024

⚠️ No Changeset found

Latest commit: 90ba2fc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jokester
Copy link

Hi I'm just a random guy investigated a few runtime configuration issues today.

For curiosity I want to ask, can it be fixed with next.js runtime config without introducing a new dependency?

@MikeShi42
Copy link
Contributor Author

@jokester I suspect it'll be an app-specific answer. One of our users opened up #498 initially so we did try that approach, he documents well the drawbacks of using the runtime config (need to disable ASO, can't use standalone builds, can't use RSC... etc). While we did use ASO (it's the default behavior in Next) and we had a build that used standalone builds - we thought we could workaround those issues.

In the end disabling ASO (by creating dummy server side props) caused issues with our <Link>-based components navigating throughout the app. I suspect disabling ASO caused navigations to get delayed, and we use nuqs liberally across the app which programmatically interfaces with the Next router. There were a lot of calls across the app from nuqs that would race the Link navigation and cause the user's navigation to cancel in favor of some other random programmatic route update from nuqs. (ex. Live Tail updating query parameters)

I couldn't see a cleaner way to disable ASO while avoiding the weird side effects it caused, so the two approaches we had left were to either go down a similar route to next-runtime-env (include a script file that's generated at run time) or a route similar to #462 (comment) where we find/replace the string at the Docker entrypoint. The former was easier to ship at the moment so we chose that path.

Hope that helps!

@jokester
Copy link

jokester commented Dec 1, 2024

So use of next-runtime-env is to enable ASO and runtime configuration at the same time? I think I got it. Thanks!!

@jokester
Copy link

jokester commented Dec 1, 2024

One related thing that confused me was the different behavior of /api/config across 2 docker images detail .

But I guess with next-runtime-env the API can even be retired so no problem 👍🏽

@@ -11,10 +9,12 @@ export const CLICKHOUSE_HOST =

// ONLY USED IN LOCAL MODE
// ex: HDX_LOCAL_DEFAULT_CONNECTIONS='[{"id":"local","name":"Demo","host":"https://demo-ch.hyperdx.io","username":"demo","password":"demo"}]' HDX_LOCAL_DEFAULT_SOURCES='[{"id":"l701179602","kind":"trace","name":"Demo Traces","connection":"local","from":{"databaseName":"default","tableName":"otel_traces"},"timestampValueExpression":"Timestamp","defaultTableSelectExpression":"Timestamp, ServiceName, StatusCode, round(Duration / 1e6), SpanName","serviceNameExpression":"ServiceName","eventAttributesExpression":"SpanAttributes","resourceAttributesExpression":"ResourceAttributes","traceIdExpression":"TraceId","spanIdExpression":"SpanId","implicitColumnExpression":"SpanName","durationExpression":"Duration","durationPrecision":9,"parentSpanIdExpression":"ParentSpanId","spanKindExpression":"SpanKind","spanNameExpression":"SpanName","logSourceId":"l-758211293","statusCodeExpression":"StatusCode","statusMessageExpression":"StatusMessage"},{"id":"l-758211293","kind":"log","name":"Demo Logs","connection":"local","from":{"databaseName":"default","tableName":"otel_logs"},"timestampValueExpression":"TimestampTime","defaultTableSelectExpression":"Timestamp, ServiceName, SeverityText, Body","serviceNameExpression":"ServiceName","severityTextExpression":"SeverityText","eventAttributesExpression":"LogAttributes","resourceAttributesExpression":"ResourceAttributes","traceIdExpression":"TraceId","spanIdExpression":"SpanId","implicitColumnExpression":"Body","traceSourceId":"l701179602"}]' yarn dev:local
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be great to update this comment as well

export const HDX_LOCAL_DEFAULT_CONNECTIONS = env(
'NEXT_PUBLIC_HDX_LOCAL_DEFAULT_CONNECTIONS',
);
export const HDX_LOCAL_DEFAULT_SOURCES = env(
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it would be handy if the NEXT_PUBLIC_IS_LOCAL_MODE is also runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave that to a separate PR, since I think we do different images/serving modes for that anyways.

wrn14897
wrn14897 previously approved these changes Dec 2, 2024
Copy link
Contributor

@wrn14897 wrn14897 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Tested it on my local and it works like a charm

@wrn14897
Copy link
Contributor

wrn14897 commented Dec 2, 2024

nit: we probably want to add public/__ENV.js to gitignore

@MikeShi42
Copy link
Contributor Author

@wrn14897 good point, updated the gitignore

@kodiakhq kodiakhq bot merged commit 4291636 into v2 Dec 2, 2024
4 checks passed
@kodiakhq kodiakhq bot deleted the mikeshi/dynamic-envs-take-two branch December 2, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants