shellscape / jsx-email

Build emails with a delightful DX
https://jsx.email
MIT License
991 stars 33 forks source link

Edge runtime #82

Open cprussin opened 10 months ago

cprussin commented 10 months ago

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/clean-css@5.3.2/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/clean-css@5.3.2/node_modules/clean-css/lib/clean.js
../../node_modules/.pnpm/clean-css@5.3.2/node_modules/clean-css/index.js
../../node_modules/.pnpm/rehype-minify-css-style@4.0.0/node_modules/rehype-minify-css-style/lib/index.js
../../node_modules/.pnpm/rehype-minify-css-style@4.0.0/node_modules/rehype-minify-css-style/index.js
../../node_modules/.pnpm/rehype-preset-minify@7.0.0/node_modules/rehype-preset-minify/index.js
../../node_modules/.pnpm/@jsx-email+render@3.0.1/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 commented 10 months ago

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.

shellscape commented 10 months ago

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

cprussin commented 10 months ago

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 commented 10 months ago

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

../../node_modules/.pnpm/uglify-js@3.17.4/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/uglify-js@3.17.4/node_modules/uglify-js/lib/ast.js
../../node_modules/.pnpm/uglify-js@3.17.4/node_modules/uglify-js/tools/node.js
../../node_modules/.pnpm/rehype-minify-event-handler@4.0.0/node_modules/rehype-minify-event-handler/lib/index.js
../../node_modules/.pnpm/rehype-minify-event-handler@4.0.0/node_modules/rehype-minify-event-handler/index.js
../../node_modules/.pnpm/rehype-preset-minify@7.0.0/node_modules/rehype-preset-minify/index.js
../../node_modules/.pnpm/@jsx-email+render@3.0.1/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.

shellscape commented 10 months ago

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

shellscape commented 10 months ago

@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 jsx-email@1.0.0-rc3 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

wooorm commented 10 months ago

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.

cprussin commented 10 months ago

@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.

shellscape commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

cprussin commented 10 months ago

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.

msereniti commented 10 months ago

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 commented 10 months ago

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.

wooorm commented 10 months ago

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

msereniti commented 10 months ago

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 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, ```
shellscape commented 10 months ago

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?

wooorm commented 10 months ago

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!

shellscape commented 10 months ago

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 commented 10 months ago

@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?

cprussin commented 10 months ago

@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!

msereniti commented 10 months ago

@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

shellscape commented 10 months ago

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

johnpyp commented 10 months ago

+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.

cloudkite commented 9 months ago

@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.

shellscape commented 9 months ago

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.

cprussin commented 9 months ago

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.

cloudkite commented 9 months ago

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.

shellscape commented 9 months ago

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

cloudkite commented 9 months ago

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
johnpyp commented 9 months ago

As far as the bundler goes, workers uses esbuild, and it doesn't look like it's tree shaking the deps. There isn't anything obvious in the workers config that would be causing tree shaking to be "disabled" in this case, esbuild does typically tree shake well like you say.

In this case, I think the tree shaking for minification and highlighting isn't happening because it's a dynamic conditional in render, rather than different imports.

Naively, maybe exporting an edgeRender function could work?

shellscape commented 9 months ago

In this case, I think the tree shaking for minification and highlighting isn't happening because it's a dynamic conditional in render, rather than different imports.

It's still not shaking out shikiji though.

A basic no-frills render is an interesting idea. Will think on that.

There's also some tsup config I can try for better optimizing of the shipped code that might allow shaking shikiji out of a bundle when unused, but minification will still be a sticking point. It's just a damn shame that edge runtimes haven't caught up. Curiously, Supabase just announced edge workers with full node compat, so maybe we'll see that trend.

johnpyp commented 9 months ago

For me at least, node compat isn't the issue. Cloudflare has quite solid node compat (no filesystem of course), it's more so the bundle-size. Bundle size is important for edge because it usually determines when you get pre-warmed to avoid cold starts. On Cloudflare, it's a 1MB gzipped threshold.

cloudkite commented 9 months ago

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

That’s the structure of source if I understand correctly. But why not maintain that structure in published version? rather than concatenating all files into index.js

shellscape commented 9 months ago

Haven't forgotten about this one, just haven't had time recently to throw at it.

@cloudkite the way that index.js/mjs is assembled, it should be tree-shakable. That's why I'm confused about the need to break up the files into chunks in /dist.

@johnpyp and @cloudkite any chance you could put together a reproduction for how you're bundling, including the specific tools you're using (even if that's Next)? https://stackblitz.com/edit/jsx-email-repro

I think what needs to happen is some bundle analyzing to figure out why it's not shaking out certain bits, and why the bundle size is so large. I really won't be able to tell until I can look through specific setups.

shellscape commented 9 months ago

Have been playing with next today to see if I could get it to create a bundle over 20mb and keep coming up unsuccessful. If anyone can drop a reproduction which does, that'll be incredibly helpful for diagnosing tree shaking problems.

cloudkite commented 8 months ago

@shellscape sorry for the delay. Heres an reproduction https://github.com/cloudkite/jsx-email-cloudflare

I'm using jsxToString directly to try avoid packages which are not edge friendly like pretty and rehype. However if you run npm run build then npm run start wrangler will complain about packages like uglify and clean-css etc

✘ [ERROR] Could not resolve "fs"

    ../node_modules/uglify-js/tools/node.js:1:17:
      1 │ var fs = require("fs");
        ╵                  ~~~~

  The package "fs" wasn't found on the file system but is built into node.
  Add the "nodejs_compat" compatibility flag to your Pages project and make sure to prefix the module name with "node:" to enable Node.js compatibility.

✘ [ERROR] Build failed with 14 errors:

  ../node_modules/clean-css/lib/options/rebase-to.js:1:19: ERROR: Could not resolve "path"
  ../node_modules/clean-css/lib/reader/apply-source-maps.js:1:17: ERROR: Could not resolve "fs"
  ../node_modules/clean-css/lib/reader/apply-source-maps.js:2:19: ERROR: Could not resolve "path"
  ../node_modules/clean-css/lib/reader/is-allowed-resource.js:1:19: ERROR: Could not resolve "path"
  ../node_modules/clean-css/lib/reader/load-original-sources.js:1:17: ERROR: Could not resolve "fs"
  ...

This didn't happen before the switch to a single jsx-email package.

shellscape commented 7 months ago

@cloudkite no worries. appreciate you putting that together. I'm working on the next major version at the moment, and this will be my next priority, will try to get compatibility into that major version.

shellscape commented 7 months ago

alright I just published a version that gets rid of uglify in the dependency tree. so that's a step in the right direction. still need to nuke clean-css.

milindgoel15 commented 7 months ago

to confirm, is the error thrown by using minify flag in the render function related to this issue?

shellscape commented 6 months ago

@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.

shellscape commented 5 months ago

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.

shellscape commented 5 months ago

@msereniti I've opened https://github.com/clean-css/clean-css/pull/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 shiki and I'm thinking about a good solution there. We may just break that out into a separate export in the package, but I've been running into bundling issues there as well.

wooorm commented 5 months ago

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 🤷‍♂️

johnpyp commented 5 months ago

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.

KyGuy2002 commented 5 months ago

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.

shellscape commented 5 months ago

@KyGuy2002 thanks for checking in. indeed, we're actively working on solutions.

benjamine commented 5 months ago

this would be awesome as it seems most other popular solutions don't support edge runtimes either (tried a few). if it helps, in the meantime I resorted to mjml+handlebars doing pre compilation at build time. I wonder if some form of pre compilation is possible for this library (compile template to output zero dependency js function), that means you get to use all the heavy dependencies at build (local, CI) and a very light and portable code on runtime.

shellscape commented 5 months ago

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 (https://github.com/shellscape/jsx-email/issues/82#issuecomment-1836854993) is probably the most robust solution for the near term. Hang in there, we're getting there.

KyGuy2002 commented 3 months ago

https://github.com/shellscape/jsx-email/issues/82#issuecomment-1836854993

I tried to install this patch using npm patch-package, but it is saying **ERROR** Failed to apply patch for package jsx-email at path with little other info.

I have never used patches before, so im sure im doing something wrong.

Anyone else able to get this working?