jsx-email icon indicating copy to clipboard operation
jsx-email copied to clipboard

Edge runtime

Open cprussin opened this issue 2 years ago • 63 comments

  • Component or Package Name: @jsx-email/render
  • Component or Package Version: 3.0.1

Expected Behavior / Situation

Compatibility with Edge runtime (vercel etc)

Actual Behavior / Situation

Not compatible with Edge runtime -- throws Module not found: Can't resolve 'os' due to clean-css:

⨯ ../../node_modules/.pnpm/[email protected]/node_modules/clean-css/lib/options/format.js:1:22
Module not found: Can't resolve 'os'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
../../node_modules/.pnpm/[email protected]/node_modules/clean-css/lib/clean.js
../../node_modules/.pnpm/[email protected]/node_modules/clean-css/index.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-minify-css-style/lib/index.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-minify-css-style/index.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-preset-minify/index.js
../../node_modules/.pnpm/@[email protected]/node_modules/@jsx-email/render/dist/index.mjs

Modification Proposal

I don't know much about what jsx-email is using clean-css for but perhaps there's an alternate library that could be used instead which is compatible with the Edge runtime and only uses web-compatible APIs?

cprussin avatar Nov 23 '23 19:11 cprussin

For what it's worth it does look like clean-css claims to be web-compatible but I don't see how the offending line (https://github.com/clean-css/clean-css/blob/cdd782ba30b84f33b7396b7128c094e7ff1cf4ba/lib/options/format.js#L1) could possibly be web compatible without bundling it with a polyfill.

cprussin avatar Nov 23 '23 19:11 cprussin

Yeah you'll probably want to take into consideration node polyfills or browserify polyfills for edge environments. We do the same for the preview app as a precaution https://github.com/shellscape/jsx-email/blob/3012648640fe3d88aede100f80c43527e557614f/packages/cli/app/vite.config.ts#L46

Let us know what you end up using and we'll add it to the docs

shellscape avatar Nov 23 '23 20:11 shellscape

I may be wrong here (I'm no expert on the edge runtime) but I don't think there's any straightforward way to add polyfills to an edge runtime environment, at least for next.js. Obviously you could add a build step that transpiles and bundles the code before sending it to the runtime, and there's a webpack bundle step somewhere under the hood in the next build which you may be able to hook into, but it doesn't look documented, stable, or intended for use for something like this.

cprussin avatar Nov 24 '23 18:11 cprussin

For what it's worth I also get the following:

../../node_modules/.pnpm/[email protected]/node_modules/uglify-js/lib/ast.js
Dynamic Code Evaluation (e. g. 'eval', 'new Function', 'WebAssembly.compile') not allowed in Edge Runtime 
Learn More: https://nextjs.org/docs/messages/edge-dynamic-code-evaluation

Import trace for requested module:
../../node_modules/.pnpm/[email protected]/node_modules/uglify-js/lib/ast.js
../../node_modules/.pnpm/[email protected]/node_modules/uglify-js/tools/node.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-minify-event-handler/lib/index.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-minify-event-handler/index.js
../../node_modules/.pnpm/[email protected]/node_modules/rehype-preset-minify/index.js
../../node_modules/.pnpm/@[email protected]/node_modules/@jsx-email/render/dist/index.mjs
./src/auth.ts

This one's a bigger issue as even with polyfills there wouldn't be a way to make eval work.

As far as I can tell, rehype is pretty solidly not edge runtime friendly.

For added context, I'm not even looking to enable the edge runtime in my next.js app -- I'm just looking to use jsx-email to generate login verification emails from next-auth. Since the next-auth config is a common config used in all next-auth functions, and in particular it gets imported into the next.js middleware to generate the authorization middleware, and since next.js middleware must run on the edge runtime, anything imported by the next-auth config must also be compatible with the edge-runtime. In other words, as long as this is an issue, jsx-email can't be used to generate auth emails with next-auth, without doing something screwy like async importing jsx-email in the email generation function (but still avoiding the edge runtime for anything other than middleware).

You could argue that next-auth's config mechanism really should allow splitting things up so we're not pulling deps into environments where they're unused, and I'd agree with you. But fixing this would be awesome regardless because it would be a shame to not be able to use jsx-email in edge environments.

cprussin avatar Nov 25 '23 06:11 cprussin

Thanks for the continued investigation. This is all.really helpful.

shellscape avatar Nov 25 '23 12:11 shellscape

@wooorm worth taking a look here. while rehype wasn't intended to be used on the edge, the prevalence of edge workers (e.g. Cloudflare) is starting to grow and this is probably going to become more of a frequently issue. at the moment it looks like clean-css and uglify are the culprits. investigating terser might be a better path forward there.

@cprussin I'll look into swapping out rehype-minify-* and adding terser back in. wanted to keep all of the actions in the rehype family, but that looks like a new can of worms (no pun intended). if you're up for joining the discord, I'm posted updates there around the move to a single package and the release candidates for those. we're currently at [email protected] with rc4 set to drop later today hopefully. Any changes to render will probably go into that, so it's worth giving a shot.

fwiw clean-css is not browser compatible out of the box. it requires extra steps which include running browserify over it. so it's not a good match for us.

Side update: Looks like html-minifier-terser also uses clean-css https://github.com/terser/html-minifier-terser/blob/c4a7ae0bd08b1a438d9ca12a229b4cbe93fc016a/package.json#L78

shellscape avatar Nov 25 '23 14:11 shellscape

I’m very open to switching to postcss/terser. Only thing I worried about before is that it shouldn’t be a humongous code increase.

wooorm avatar Nov 25 '23 14:11 wooorm

@shellscape yeah I'm happy to hop on the discord. I'm also more than happy to contribute some PRs in the appropriate places to help move this along, but I'm not familiar with rehype beyond what I've seen here so at the very least I'd need pointers on where the appropriate place to fix is.

cprussin avatar Nov 25 '23 15:11 cprussin

On the rehype side, anywhere uglifyjs is used, terser could be plugged in. Where clean-css is used, https://github.com/cssnano/cssnano could be used. It looks like the only place in the rehype ecosystem those are used is in https://github.com/rehypejs/rehype-minify.

I've also opened this issue as a fallback https://github.com/terser/html-minifier-terser/issues/174

shellscape avatar Nov 25 '23 15:11 shellscape

I reached out to the clean-css author and heard back. He's open to a quick fix for the os require, so hopefully we can tick that one off without much fuss. That leaves swapping out uglifyjs

shellscape avatar Nov 28 '23 15:11 shellscape

jsx-email v1.0.1 is out. still waiting on the publish for clean-css, but I'm being patient there.

shellscape avatar Nov 30 '23 04:11 shellscape

awesome, thanks @shellscape . The soonest I'll get time to hack on this would be this weekend, I'll try to get a PR up to swap out terser for uglifyjs then.

cprussin avatar Nov 30 '23 15:11 cprussin

Dynamic require of os in clean-css is just tip of the iceberg. The https://github.com/clean-css/clean-css/pull/1262 still doesn't allow to use clean-css in the edge runtime as far as it contains a lot of fs, path, http, https and url imports. While any of that import is not dynamic this package cannot be used in the edge runtime.

You either need to pick less platform dependent css minifier or put enormous work on making clean-css edge compatible.

msereniti avatar Dec 01 '23 21:12 msereniti

Also, I must add that to make the library work on edge runtime alongside the incompatible dependencies of rehype-preset-minify you need to deal with shikiji https://github.com/shellscape/jsx-email/blob/b8b44ff7472f13698f2674d53892402f6f6269a7/packages/jsx-email/src/components/code.tsx#L3C1-L3C1. It's also doesn't work on edge runtime.

Remember that when you are trying to run an application on edge runtime you always see only one dependency that ruins everything. So you mast one-by-one comment/remove each of them to get the whole list what breaks the edge runtime.

msereniti avatar Dec 01 '23 21:12 msereniti

starry-night should work everywhere, and be similar enough to shikiji

wooorm avatar Dec 01 '23 22:12 wooorm

Here is a pnpm patch for version 1.0.3 that (while of course breaking minify and <Code /> functionality makes the react-jsx work on the edge

the patch is here
diff --git a/dist/index.js b/dist/index.js
index 2d71704a29b54afb2b595f2a347baef1d7882a73..35c7dba9ed2a07e33943f1450e208c6c7b6ffbce 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -230,7 +230,7 @@ Button.displayName = "Button";
 // src/components/code.tsx
 var import_p_memoize = __toESM(require("p-memoize"));
 var import_react = require("react");
-var import_shikiji = require("shikiji");
+// var import_shikiji = require("shikiji");
 
 // src/render/jsx-to-string.ts
 var import_hash_it = __toESM(require("hash-it"));
@@ -541,13 +541,13 @@ __name(isReactForwardRef, "isReactForwardRef");
 
 // src/components/code.tsx
 var import_jsx_runtime3 = require("react/jsx-runtime");
-var getHighlighter = (0, import_p_memoize.default)(async (language, theme = "nord") => {
-  const shiki = await (0, import_shikiji.getHighlighter)({
-    langs: language ? [language] : [],
-    themes: [theme]
-  });
-  return shiki;
-});
+// var getHighlighter = (0, import_p_memoize.default)(async (language, theme = "nord") => {
+//   const shiki = await (0, import_shikiji.getHighlighter)({
+//     langs: language ? [language] : [],
+//     themes: [theme]
+//   });
+//   return shiki;
+// });
 var Renderer = /* @__PURE__ */ __name((props) => {
   const { children, language, style, theme = "nord", ...rest } = props;
   const highlighter = useData(props, () => getHighlighter(language, theme));
@@ -981,10 +981,10 @@ var processHtml = /* @__PURE__ */ __name(async ({ html, minify, pretty, strip })
   let processor = rehype().data("settings", settings).use(rehypeMoveStyle);
   if (strip)
     processor = processor.use(rehypeRemoveDataId);
-  if (minify) {
-    const { default: rehypeMinify } = await import("rehype-preset-minify");
-    processor = processor.use(rehypeMinify);
-  }
+  // if (minify) {
+  //   const { default: rehypeMinify } = await import("rehype-preset-minify");
+  //   processor = processor.use(rehypeMinify);
+  // }
   const doc = await processor.use(stringify, {
     allowDangerousCharacters: true,
     allowDangerousHtml: true,
diff --git a/dist/index.mjs b/dist/index.mjs
index 5e06dd9a26084cf09e692b71ab8ad7921311e9e3..1a5c7f520dac9c91f192dd27fd2d25c68f52927f 100644
--- a/dist/index.mjs
+++ b/dist/index.mjs
@@ -169,7 +169,7 @@ Button.displayName = "Button";
 // src/components/code.tsx
 import mem from "p-memoize";
 import { Suspense } from "react";
-import { getHighlighter as getHighBro } from "shikiji";
+// import { getHighlighter as getHighBro } from "shikiji";
 
 // src/render/jsx-to-string.ts
 import hash from "hash-it";
@@ -480,13 +480,13 @@ __name(isReactForwardRef, "isReactForwardRef");
 
 // src/components/code.tsx
 import { Fragment, jsx as jsx3 } from "react/jsx-runtime";
-var getHighlighter = mem(async (language, theme = "nord") => {
-  const shiki = await getHighBro({
-    langs: language ? [language] : [],
-    themes: [theme]
-  });
-  return shiki;
-});
+// var getHighlighter = mem(async (language, theme = "nord") => {
+//   const shiki = await getHighBro({
+//     langs: language ? [language] : [],
+//     themes: [theme]
+//   });
+//   return shiki;
+// });
 var Renderer = /* @__PURE__ */ __name((props) => {
   const { children, language, style, theme = "nord", ...rest } = props;
   const highlighter = useData(props, () => getHighlighter(language, theme));
@@ -920,10 +920,10 @@ var processHtml = /* @__PURE__ */ __name(async ({ html, minify, pretty, strip })
   let processor = rehype().data("settings", settings).use(rehypeMoveStyle);
   if (strip)
     processor = processor.use(rehypeRemoveDataId);
-  if (minify) {
-    const { default: rehypeMinify } = await import("rehype-preset-minify");
-    processor = processor.use(rehypeMinify);
-  }
+  // if (minify) {
+  //   const { default: rehypeMinify } = await import("rehype-preset-minify");
+  //   processor = processor.use(rehypeMinify);
+  // }
   const doc = await processor.use(stringify, {
     allowDangerousCharacters: true,
     allowDangerousHtml: true,

msereniti avatar Dec 01 '23 22:12 msereniti

Thanks for all of this investigation folks. A few notes that I want to add to the discussion:

  • one of the reasons that shikiji was chosen was that the vite ecosystem has heavily centered around it, it's widely used now, and the themes are familiar. while the code component isn't widely used as yet, that's still a compelling reason
  • shikiji was chosen because of it's increased speed over shiki
  • minification is a crucial component of the package, and is important when trying to fit a large amount of content into an email and getting it under the 102kb above-the-fold limit that gmail imposes. (half that on gmail mobile)

@wooorm starry-night looks really well done, thanks for sharing. are there any plans to increase the number of default themes?

shellscape avatar Dec 02 '23 06:12 shellscape

No. It uses classes, so you can use any CSS you want. Or you can rewrite the AST to result in anything you want. The CSS it ships is the CSS that GitHub ships to make it exactly like them. Nothing else!

wooorm avatar Dec 04 '23 15:12 wooorm

OK thanks for that. For onlookers, we'd need to at minimum have equivalent themes for starry-night as what shikiji ships with before we could migrate.

shellscape avatar Dec 04 '23 16:12 shellscape

@msereniti @cprussin what would you think of a leaner @jsx-email/edge package that contained only the components and render which did none of the minification and didn't contain the Code component?

shellscape avatar Dec 08 '23 18:12 shellscape

@shellscape it's not a bad stopgap and it would solve all my current problems, but it's also not an ideal solution -- I do want minification and if I ever had a use case for sending out code samples in email I'd want them to be highlighted too, and it would be a shame to not be able to do that in an edge environment.

But at least doing a @jsx-email/edge would enable the library to be usable in those environments at all, which currently it is not, and if eventually the minification and Code stuff were made compatible then it would be a great upgrade. So it's strictly better than where we are today.

Sorry, I still haven't found any time to contribute back here. I really appreciate the time & effort you've been putting in to help with this!

cprussin avatar Dec 08 '23 19:12 cprussin

@shellscape It will not be much better than package patching like I've provided above. Minification (especially) and code highlighting are very important features for emails generating.

Sorry, I also doesn't have time to contribute into the package for now. Overall it's great and no edge-runtime capability seems to be the only major issue

msereniti avatar Dec 09 '23 14:12 msereniti

Thanks for the feedback on that. Will continue to look for a happier path there.

shellscape avatar Dec 09 '23 14:12 shellscape

+1 on this. Shikiji itself seems like it can be used by cloudflare workers (based on their docs, and this is the runtime I'm using), but the bundle size addition makes it close to a non-starter for most edge runtimes as it adds 6.6 MB to final bundle, making it by far the largest dependency in my whole graph and more than 60% of my final build size.

An @jsx-email/edge build without minification and shikiji would solve most of my issue, but of course like others commented if an alternative for minification in the future could be found that would be ideal.

johnpyp avatar Dec 11 '23 06:12 johnpyp

@shellscape How about making the packages that where previously available via "@jsx-email/code" available as well via sub paths within the same package "jsx-email/code" etc

This would allow people that want to use jsx-email in edge runtimes to avoid incompatible components like code or tailwind.

For example I could then use jsx-email in cloudflare workers by not importing from the package index file "jsx-email" and import respective parts I need instead ie "jsx-email/render", "jsx-email/button", "jsx-email/heading" etc.

Also has the added benefit that I don't need to load the parts of jsx-email that I do not use since bundle size can be an issue in edge runtimes.

cloudkite avatar Dec 13 '23 22:12 cloudkite

One of the reasons we moved to a single package was due to circular package dep issues with publishing, and it's not something I'd like to wade back into. The juice isn't worth the squeeze.

wrt bundling, any bundler worth their salt should be tree shaking unused code away, so that really shouldn't be a concern. If that's not happening, then we need to take a deeper look.

shellscape avatar Dec 13 '23 23:12 shellscape

I'd also argue the point I made above, where even if we did split packages it's a half baked solution because minification and code highlighting are perfectly reasonable things to want to do from an edge environment.

cprussin avatar Dec 14 '23 00:12 cprussin

To be clear I meant still have a single package but have multiple files within that single package ie index.js render.js code.js ...

index.js could just re-export from other files.

cloudkite avatar Dec 14 '23 00:12 cloudkite

I'm confused; that's the structure we have now

shellscape avatar Dec 14 '23 01:12 shellscape

I'm confused; that's the structure we have now

But the published package only includes index.js and index.mjs?

CleanShot 2023-12-14 at 14 01 53@2x

cloudkite avatar Dec 14 '23 01:12 cloudkite