Edge runtime #82
Comments
|
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. |
|
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 jsx-email/packages/cli/app/vite.config.ts Line 46 in 3012648 Let us know what you end up using and we'll add it to the docs |
|
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. |
|
For what it's worth I also get the following: 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. |
|
Thanks for the continued investigation. This is all.really helpful. |
|
@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 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 |
|
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. |
|
@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. |
|
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 terser/html-minifier-terser#174 |
|
I reached out to the clean-css author and heard back. He's open to a quick fix for the |
|
jsx-email v1.0.1 is out. still waiting on the publish for clean-css, but I'm being patient there. |
|
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. |
|
Dynamic require of You either need to pick less platform dependent css minifier or put enormous work on making clean-css edge compatible. |
|
Also, I must add that to make the library work on edge runtime alongside the incompatible dependencies of 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. |
|
|
|
Here is a pnpm patch for version the patch is herediff --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, |
|
Thanks for all of this investigation folks. A few notes that I want to add to the discussion:
@wooorm starry-night looks really well done, thanks for sharing. are there any plans to increase the number of default themes? |
|
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! |
|
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. |
|
@msereniti @cprussin what would you think of a leaner |
|
@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 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! |
|
@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 |
|
Thanks for the feedback on that. Will continue to look for a happier path there. |
|
+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 |
|
@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. |
|
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. |
|
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. |
|
To be clear I meant still have a single package but have multiple files within that single package ie index.js could just re-export from other files. |
|
I'm confused; that's the structure we have now |
|
alright I just published a version that gets rid of |
|
to confirm, is the error thrown by using minify flag in the render function related to this issue? |
|
@milindgoel15 it's one of them, yes. I've arrived on a good solution (I think) for this for any sort of environment. Will have more on this next week. |
|
Still making progress on this. We have a plugins system implemented that will allow people to optionally include different phases of post-processing (or none at all) which should take care of the immediate problems outlined in this issue. Next phase will be to make those dependencies edge compatible so those plugins can be used on the edge. |
|
@msereniti I've opened clean-css/clean-css#1273 to tackle the other node-builtin requires. Implementing terser in the rehype minify plugin would 4x the underlying dependency in both bundling and dependency install - something that I don't think @wooorm is interested in as that would qualify as a humungus increase. Still the issue of |
|
I have worried about it before, but it makes sense that a good minifier includes a ton of code to shave off a few bytes. So right now I’m 🤷♂️ |
|
As a temporary solution I just forked and removed the Code element and the other heavy bundle things. I couldn't seem to get Esbuild to treeshake it away, not sure why. |
|
I am also running into this issue from react-email. This looks really appealing since it seems to be a drop in replacement for react-email with a better community/support. And this issue is active. I would really like to get this to work on cloudflare pages functions, even if it's just a temporary/rough workaround with missing features or optimizations. |
|
@KyGuy2002 thanks for checking in. indeed, we're actively working on solutions. |
|
this would be awesome as it seems most other popular solutions don't support edge runtimes either (tried a few). |
|
Precompilation isn't something I'd want to burn time on, preferring to use what time I have for a long term solution. What @johnpyp did with a fork or @msereniti did (#82 (comment)) is probably the most robust solution for the near term. Hang in there, we're getting there. |
|
I tried to install this patch using npm patch-package, but it is saying I have never used patches before, so im sure im doing something wrong. Anyone else able to get this working? |
|
Coming from react-email, I'm using cloudflare pages with next-on-pages for my nextjs app, react-email/tailwind package are too large until can't deploy it in cloudflare pages. So I switch to jsx-email, after amending the code to fit jsx-email, I hit a lot of errors from clean-css: Any idea on how to fix this? |
|
@sisheng1998 this entire issue is about getting the package to work correctly on the edge. I suggest you read the entire thread. |
|
the v2.0.0 PR is up and gets us closer to where we want to be for edge compat #206 this will let you optionally include the minify plugin. i'm planning a cloudflare integration test following this release (thanks to our friends at SST) which will help guarantee edge compat moving forward. |
|
v2 release will happen around 1PM ET tomorrow. A Cloudflare recipe and direct support will be my top priority afterwards. |
|
v2 is out the door and we've got the major version bumps behind us. This is now first up on my priority list. |
|
Seems that Cloudflare has added a lot more compatibility for Workers: https://blog.cloudflare.com/more-npm-packages-on-cloudflare-workers-combining-polyfills-and-native-code/. Will be testing this out pronto. |
|
Hi, is there a current workaround or working up-to-date patch available? |
|
@JonasDoesThings Cloudflare has greatly improved their node support in wrangler since this issue was opened and jsx-email works within a worker now. I just haven't had time to publish the recipe for a sample setup, so the issue remains open. |
|
Unfortunately that doesn't help the bundle size issue afaict, which is a major concern for latency on workers. A lite-build that excludes Code (and the dependencies which Code depends on) would be great from that perspective. I'm not sure why the tree shaking doesn't work in workers for it. |
|
@shellscape thx for the quick reply. Maybe the issue is on my end then, but I'm using next 15.1.6 and jsx-email 2.7.0, and getting the following error when trying to use the library (in Do you have an idea? |
|
The Vercel edge runtime is a whole other ballgame too, which looks to be what you're running into. |
|
Ahh I think, it's caused by me importing my sendEmail function (that imports jsx-email's render fn) in my auth.ts file, which is also imported in my middleware to check the user's authentication status. The middleware has even less nodejs APIs available, so maybe I could split that up, or only dynamically import jsx-email when needed - when it's not running in the middleware. So probably my fault, and not jsx-email's 😅 |
|
@JonasDoesThings that does give me an idea though. I can probably make shiki an optional peer dependency. we are already dynamically importing that and I can wrap it in a catch to print a message if the dependency is not installed, and that should get around bundlers which have poor tree shaking. I'll see about adding that this week. |
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 toclean-css:Modification Proposal
I don't know much about what jsx-email is using
clean-cssfor 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?The text was updated successfully, but these errors were encountered: