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

svgo and SSR #35

Closed
JuanM04 opened this issue May 6, 2022 · 35 comments · Fixed by #111
Closed

svgo and SSR #35

JuanM04 opened this issue May 6, 2022 · 35 comments · Fixed by #111
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@JuanM04
Copy link

JuanM04 commented May 6, 2022

svgo is a Node.js-only package, thus, it doesn't work in the brand-new environments that Astro SSR supports (e.g. Netlify/Vercel Edge, Deno, Cloudflare workers).

Is there any other option that supports more environments? Or there could be a way to disable svgo altogether so Astro can build successfully?

@JReinhold
Copy link

JReinhold commented Jun 20, 2022

SVGO and SSR

To add more context to the issue, the error that is thrown when building is this:

22:48:08 [build] Building SSR entrypoints...
[commonjs] Cannot bundle Node.js built-in "os" imported from "node_modules/svgo/lib/svgo-node.js".
Consider disabling ssr.noExternal or remove the built-in dependency.

Many of the SSR adapters enable noExternal in the vite ssr configuration (for good reason). I've tried to remove that by patching the integration module, but that just results in other errors:

 error   Build failed with 4 errors:
  node_modules/svgo/lib/svgo-node.js:3:19: ERROR: Could not resolve "os"
  node_modules/svgo/lib/svgo-node.js:4:19: ERROR: Could not resolve "fs"
  node_modules/svgo/lib/svgo-node.js:5:34: ERROR: Could not resolve "url"
  node_modules/svgo/lib/svgo-node.js:6:21: ERROR: Could not resolve "path"

Fetching SVGs and SSR

Another issue with SSR is that the default behaviour of astro-icon is to fetch the icon via the web service. This makes sense when building a static site, but in SSR mode it would mean we make requests to the web service all the time, which I don't think makes sense. (I could be wrong about all this though, this is just what I gathered from the source code.)

I'm not sure how to fix all of this. It seems like the perfect solution would be to make the SVG fetching and optimisation happen at the build step instead of at runtime, but looking at the Astro Integration API docs I'm not sure we can hook into that and change the source code during build? @natemoo-re
If this is in fact possible though, I'm open to do a PR for it.

@ubarilan
Copy link

I have the exact same issue.
This feature is necessary because many people run astro with ssr in mind and it makes that impossible when using astro-icon

@jjjrmy
Copy link

jjjrmy commented Aug 19, 2022

Having the same issue when trying to use with Cloudflare Pages

@simpleauthority
Copy link

Resolution / update badly needed. Cannot deploy to CF pages using SSR :-(

@simpleauthority
Copy link

After doing some research, I have found that svgo actually includes a browser version as well as a node version. I have feebly tried to fork and create a PR but I cannot get it working properly on my dev server. Node complains about unexpected exports, even though a diff between the versions shows only some work in the optimize function...

Sigh.

Anyway, one can import optimize from svgo for the node-js version or one can import optimize from svgo/dist/svgo.browser for the browser version. Can someone better than I possibly capitalize on this to help get this project in the proper direction for ssr?

My sad attempt is here, and for whatever reason I can't get this to run on my dev server. In any case, it results in the same issue at CF where it can't bundle the node version. Maybe distributing two separate packages??

https://github.com/simpleauthority/astro-icon/tree/feat/ssr-support

@mrabtikhalid
Copy link

mrabtikhalid commented Aug 25, 2022

hello @simpleauthority i opened an issue #52 and i suggested to use a javascript native approach and not depend on nodejs , i can help with a fix , i just need some hints or approach, i will search for alternatives later

@simpleauthority
Copy link

hello @simpleauthority i opened an issue #52 and i suggested to use a javascript native approach and not depend on nodejs , i can help with a fix , i just need some hints or approach, i will search for alternatives later

Yeah I am not sure if there is anything that is plain JS or not. I'll have to keep looking, too.

@pedrosc94
Copy link

pedrosc94 commented Aug 30, 2022

I also can't deploy on netlify with edge functions.
I know it's not ideal because the svg won't get optimized, but for temporary solution removing all the svgo related stuff wouldn't fix the problem? I just don't know how to do it.

@simpleauthority
Copy link

That would work but not sure if it would be an acceptable solution for @natemoo-re. It might be as simple as commenting out this line https://github.com/natemoo-re/astro-icon/blob/main/packages/core/lib/utils.ts#L83 for now but not sure if we'd need to go and actually remove all the code for it to get it to run properly.

@mrabtikhalid
Copy link

I endup using the library without the svg compression for the moment,

@pedrosc94
Copy link

Yeah I also went for a workaround for now, and created a RootLayout.astro with <script src="https://cdnjs.cloudflare.com/ajax/libs/iconify/3.0.0-beta.3/iconify.min.js"></script> and then I just import that layout to my other layouts, then I use <span class="iconify-inline" data-icon="simple-icons:twitter" data-height="24"></span>

@pedrosc94
Copy link

But would would prefer to use this package just for the sake of simplicity

@cami7ord
Copy link

Same problem deploying to deno.

@lasfito
Copy link

lasfito commented Sep 16, 2022

bumping this up!

@skreutzberger
Copy link

I have the same problem with SSR on Netlify.

@mrabtikhalid
Copy link

I think we should separate the svg compression , and each server library (netlify/deno....) should implement its own compression library, we should add just an interface for compression

@jjjrmy
Copy link

jjjrmy commented Sep 27, 2022

Is there a workaround for the time being?

SenseiMarv added a commit to SenseiMarv/the-frontview that referenced this issue Oct 14, 2022
* Build: Switch to static Vercel deployment

This change is required to work around natemoo-re/astro-icon#35.
SenseiMarv added a commit to SenseiMarv/the-frontview that referenced this issue Oct 14, 2022
* Chore: Move from astro-icon to iconify-icon

This change is required to work around natemoo-re/astro-icon#35.
@stramel
Copy link
Collaborator

stramel commented Oct 20, 2022

This definitely seems like something we should work to improve upon. I'm not sure what the best route is yet but did notice that there is a browser version that we could import which should allow it to work with most environments.

@stramel stramel added bug Something isn't working help wanted Extra attention is needed labels Oct 20, 2022
@stramel
Copy link
Collaborator

stramel commented Oct 21, 2022

I was looking into this and think we could utilize the browser version of svgo and then allow for specifying a custom optimizer in a file like src/icons/_optimize.ts. Does this sound like it would be useful?

@simpleauthority
Copy link

I tried to fork astro-icon to use the browser version before, but was unsuccessful. If you can figure out how to get that to work, then yes!

@stramel
Copy link
Collaborator

stramel commented Oct 24, 2022

Will be working to shift svgo over to build time optimization and only apply it to local icons.

@rupampoddar
Copy link

rupampoddar commented Oct 26, 2022

Figured out a simple workaround (thanks to @simpleauthority 's comment).

We need to patch the astro-icon package. More specifically we need to update line 5 of lib/utils.ts .

I'm using pnpm and it has built-in patching feature.
Read this: https://pnpm.io/cli/patch (watch the video on that page)

(Note: yarn v2+ also has built-in patching feature, for npm you could use https://github.com/ds300/patch-package)

Step 1: run pnpm ls to find out the version

image

Step 2: run pnpm patch astro-icon@0.7.3 (adjust version number)

image

Step 3: open the /tmp/... folder in VS Code or any other editor

code /tmp/24c156128bab6fdefc8998671638ad98 &

step 4: edit lib/utils.ts and Save the file

// change line 5 from this
import { optimize as optimizeSVGNative } from "svgo";

// to this
import { optimize as optimizeSVGNative } from "svgo/dist/svgo.browser";

image

Step 5: commit the patch using pnpm patch-commit command

# example (adjust the folder path)
pnpm patch-commit /tmp/24c156128bab6fdefc8998671638ad98

Done

Run the build command

image

Update

Although astro build is working with the patch, astro dev is now throwing error. So I reverted that patch for now.

@stramel
Copy link
Collaborator

stramel commented Oct 26, 2022

Looked into this today and it seems there will need to be some effort put into getting this to SSR properly in non-node environments. Will be working on this as a top priority.

@demiro
Copy link

demiro commented Oct 31, 2022

I tried the patch @rupampoddar suggested, but I am getting this error

image

image

what is the svgo version that you are using?

it looks if I remove svgo from the dependencies, it works

@rupampoddar
Copy link

rupampoddar commented Oct 31, 2022

Hi @demiro ,
Have you added external: ["svgo"] in astro config ?

Here's my astro.config.mjs file:

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

export default defineConfig({
  output: "server",
  integrations: [
    // ...
  ],
  adapter: cloudflare({
    mode: "directory",
  }),
  vite: {
    define: {
      // ...      
    },
    ssr: {
      external: ["svgo"], // <=== THIS
    },
  },
});

Update on the patch

Currently my patched file (astro-icon@0.8.0 - lib/utils.ts) looks like the following. Both build and dev is working.
(Note: I am not using any local icon files)

/// <reference types="vite/client" />
import { SPRITESHEET_NAMESPACE } from "./constants";
import { Props, Optimize } from "./Props";
import getFromService from "./resolver";
// import { optimize as optimizeSVGNative } from "svgo";    // <==== (PATCH) comment out or remove line

// Adapted from https://github.com/developit/htmlParser
const splitAttrsTokenizer = /([a-z0-9_\:\-]*)\s*?=\s*?(['"]?)(.*?)\2\s+/gim;
const domParserTokenizer =
  /(?:<(\/?)([a-zA-Z][a-zA-Z0-9\:]*)(?:\s([^>]*?))?((?:\s*\/)?)>|(<\!\-\-)([\s\S]*?)(\-\->)|(<\!\[CDATA\[)([\s\S]*?)(\]\]>))/gm;

const splitAttrs = (str) => {
  let res = {};
  let token;
  if (str) {
    splitAttrsTokenizer.lastIndex = 0;
    str = " " + (str || "") + " ";
    while ((token = splitAttrsTokenizer.exec(str))) {
      res[token[1]] = token[3];
    }
  }
  return res;
};

// (PATCH) update this function
function optimizeSvg(
  contents: string,
  name: string,
  options: Optimize
): string {
  return contents;    // <==== return content without doing anything,
}

PNPM & Cloudflare Pages

When using pnpm, you also need to make some changes to the build configuration on cloudflare. Follow this : https://community.cloudflare.com/t/add-pnpm-to-pre-installed-cloudflare-pages-tools/288514/5

@steveninety
Copy link

@rupampoddar Thank you for your efforts! I can confirm that your solution works with local svg's too :)

@FlipFloop
Copy link

Uhm is this still failing for anyone else using pnpm with vercel?

@josemarcilio
Copy link

josemarcilio commented Mar 27, 2023

@rupampoddar thank you, so much. This is working for me with Vercel, and also for local icons

@herbras
Copy link

herbras commented May 20, 2023

Thanks very helpful

@stramel stramel added this to the v1 milestone May 25, 2023
@hongfanmeng
Copy link

Here is a way without using pnpm patch, you can configure the vite compilation option of astro.config.mjs

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  vite: {
    resolve: {
      alias: {
        "svgo": import.meta.env.PROD ? "svgo/dist/svgo.browser.js" : "svgo"
      }
    }
  }
});

@stramel stramel mentioned this issue Jun 6, 2023
@stramel
Copy link
Collaborator

stramel commented Jun 29, 2023

Feel free to try out the next release which has support for SSR and svgo v3! You can try it out with:

npm i astro-icon@next

and read how to use it here: https://github.com/natemoo-re/astro-icon/blob/v1/packages/core/README.md

@twodft
Copy link

twodft commented Sep 6, 2023

Here is a way without using pnpm patch, you can configure the vite compilation option of astro.config.mjs

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  vite: {
    resolve: {
      alias: {
        "svgo": import.meta.env.PROD ? "svgo/dist/svgo.browser.js" : "svgo"
      }
    }
  }
});

Great thanks!!! this workaround works.

@geekyayush
Copy link

Here is a way without using pnpm patch, you can configure the vite compilation option of astro.config.mjs

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  vite: {
    resolve: {
      alias: {
        "svgo": import.meta.env.PROD ? "svgo/dist/svgo.browser.js" : "svgo"
      }
    }
  }
});

You are a life saver. Thank you!

jbmoelker added a commit to voorhoede/head-start that referenced this issue Nov 5, 2023
removed astro-icon since Cloudflare setup requires hybrid setup, but astro-icon uses svgo which relies on native nodejs modules not available in Cloudflare workers. See natemoo-re/astro-icon#35 .
@salloom-domani
Copy link

salloom-domani commented Nov 19, 2023

Here is a way without using pnpm patch, you can configure the vite compilation option of astro.config.mjs

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  vite: {
    resolve: {
      alias: {
        "svgo": import.meta.env.PROD ? "svgo/dist/svgo.browser.js" : "svgo"
      }
    }
  }
});

didnt work for me, when building the project it errors the following
"Named export 'optimize' not found. The requested module 'svgo/dist/svgo.browser.js' is a CommonJS module, which may not support all module.exports as named exports"

@vtisnado
Copy link

vtisnado commented Feb 6, 2024

Here is a way without using pnpm patch, you can configure the vite compilation option of astro.config.mjs

import { defineConfig } from "astro/config";
import cloudflare from "@astrojs/cloudflare";

// https://astro.build/config
export default defineConfig({
  output: "server",
  adapter: cloudflare(),
  vite: {
    resolve: {
      alias: {
        "svgo": import.meta.env.PROD ? "svgo/dist/svgo.browser.js" : "svgo"
      }
    }
  }
});

didnt work for me, when building the project it errors the following "Named export 'optimize' not found. The requested module 'svgo/dist/svgo.browser.js' is a CommonJS module, which may not support all module.exports as named exports"

This also didn't worked for me. Still getting the Could not resolve "fs" error even with the latest AstroWind (1.0.0-beta.17) and astro-icon (^1.0.3)

Any clue in how to solve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.