feat: generate script text at build time

[dario-piotrowicz]

Fixes #349

[dario-piotrowicz]

Note: if there is a concern about loosing typesafety here this solution can be improved by either using jsdoc type comments here or adding a custom esbuild loader to the tsup config,

[eposha]

Is it possible to review and merge?

[vicb]

@pacocoursey The PR would help with the Cloudflare adapter for OpenNext - is there anything missing from the PR before it can be merged?
🙏

[pacocoursey]

Sorry, I'm late to this PR. It looks important to fix.

@dario-piotrowicz @vicb Can you share more details on the root issue here? The problem is calling .toString() on a Function instance in order to print its code?

The reason this is useful is that the code for the inline script (currently script.ts) is transpiled/minified alongside the rest of next-themes, meaning we inject cleaner/faster/production code into pages instead of the raw script.

// script.ts => 
var I = (e, t, n, l, r, a, u, h) => {
  // …vanilla JS script contents…
};

In your version, the entire script is un-minified and raw.

var L=`(
  attribute,
  storageKey,
  defaultTheme,
  forcedTheme,
  themes,
  value,
  enableSystem,
  enableColorScheme
) => ...

This is why I currently use script.toString().

[vicb]

The problem we have is when the next-themes code is the server code.

OpenNext uses ESBuild to bundle and minify the server code. ESBuild would minify next-themes code. As part of that it renames functions and add a call to __name(rf, "originalFnName") where rf is the renamed function.

The code is valid in the server bundle where ESBuild generates a global __name function. But running the function body on the browser where __name is undefined crashes.

Providing the code as a string would prevent it to be modified during the server build step.

I think that the solution would be for this PR to minify the code.

[trm217]

Why not implement this as an option rather than it being the default behavior?

[vicb]

Why not implement this as an option rather than it being the default behavior?

It could certainly be an option.

That being said what would be the use case for keeping the current behavior if the code is available as a string in it minified version?

IMO having an option for the string to be minified or not could be more helpful?

[dario-piotrowicz]

Hi @pacocoursey thanks for you comment, no worries for the late review, better late than never 🫶

Is the minification the only concern you have with this solution? Would you be ok if the built-time generated script would be minified?

PS: regarding @trm217 suggestion, sorry but I personally am not a fan of adding an option for this, as I think that this is more of an implementation detail than something the user should actually be aware/concerned of, or am I missing something? 🤔

[trm217]

@dario-piotrowicz I'd argue the simplest solution should be the default case. All other advanced use cases should be able to be solved with an extendible API. Not the other way arround. I agree that it's an implementstion detail. But as paco mentioned, there are benefits to the current solution.

[dario-piotrowicz]

But as paco mentioned, there are benefits to the current solution.

Could you clarify that benefits there are to the current solution? because my understanding here is that the only benefit is the minification of the code and as I asked in my previous comment I think that we can just include that in the built-time generation of the script, assuming that both solutions include the script minification, what other benefits would come from the runtime generation of the script?

[mnove]

I think this is causing me some next-theme issues when deploying a nextjs app with Cloudflare adapter for OpenNext (v1)

The temp fixed I found is to update your wrangler.jsonc and add keep_names: false.
See: https://opennext.js.org/cloudflare/howtos/keep_names#how-to-fix-such-issues

I haven't tested yet if this change causes other issues.

[man-bug]

+1

keep_names: false is a fine workaround for now, but if a fix for the library is available, that feels like the intuitive choice.

This does not have to be default behavior; an optional prop (similar to the existing fix for Cloudflare Rocket Loader) would be awesome.

Was hinted at before with vicb's comment on 'crashes,' but to be clear, this issue seems to cause silent SSR failure, not just a light mode flash!