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

Error including bytemd dependency #281

Closed
benmccann opened this issue Feb 26, 2022 · 17 comments · Fixed by #282
Closed

Error including bytemd dependency #281

benmccann opened this issue Feb 26, 2022 · 17 comments · Fixed by #282
Labels
bug Something isn't working

Comments

@benmccann
Copy link
Member

benmccann commented Feb 26, 2022

Describe the bug

Someone on Discord reported that they can't get bytemd to work. It looks to me like vite-plugin-svelte is not detecting that bytemd's lodash.debounce dependency needs to be prebundled. There are also issues reported in the bytemd repo and on Discord where the error occurs on other dependencies

A couple things I noticed that are off about the packaging:

  • bytemd uses .esm.js and .cjs.js extensions instead of .mjs and .cjs
  • lodash.debounce has no main or any other entry point specified in the package.json

Even when I manually edit those I still get the same error.

Reproduction

https://github.com/benmccann/sveltekit-bytemd-bug-reproduction

Logs

8:07:24 PM [vite] Error when evaluating SSR module /node_modules/lodash.debounce/index.js?v=dc88d06f:
ReferenceError: module is not defined
    at /node_modules/lodash.debounce/index.js?v=dc88d06f:377:1

System Info

reproduction provided

Severity

The top two bugs in the bytemd repo are about how the project doesn't work with SvelteKit: https://github.com/bytedance/bytemd/issues/96 and https://github.com/bytedance/bytemd/issues/139

@benmccann benmccann added bug Something isn't working triage Awaiting triage by a project member labels Feb 26, 2022
@benmccann
Copy link
Member Author

I don't think this is actually fixed unfortunately. I just upgraded to the latest and am still seeing the same error

@bluwy bluwy reopened this Feb 27, 2022
@bluwy
Copy link
Member

bluwy commented Feb 27, 2022

My bad. I assumed this was a pre-bundling bug and only fixed that, looks like the error is coming from loading the module in SSR 🤔

@bluwy
Copy link
Member

bluwy commented Feb 27, 2022

TL;DR: ssr.noExternal on a dep considers the dep to be source code (Everything is ESM including its transitive deps, like pre-bundling)


Wow this seems like a huge deal than I expected. Through some testing, I realized the issue is that ssrLoadModule is called on lodash.debounce, but that doesn't work because ssrLoadModule works for ESM code only. Ideally Vite expects this to never happen in the first place.

For our case, it happens because ssrLoadModule is called by ssrImport, which happens when you do import debounce from "lodash.debounce" in your code. And in this case, bytemd's is doing so.

However, this isn't an issue in most cases, as long as lodash.debounce is externalized, which Vite does so by default. When it is externalized, Vite's import analysis will keep the import as is, so you get this in Vite's complex SSR transformation:

const debounce = __vite_ssr_import__("lodash.debounce")

If not externalized, you get:

const debounce = __vite_ssr_import__("/node_modules/lodash.debounce/index.js")
// notice this is the path of the error message of the issue

The different path is important as it gets resolved differently by ssrImport.

In Svelte's case, this is not externalized because bytemd is Svelte library, and it's noExternal by default, which means any deps below it is not externalized, including lodash.debounce.

A solution is to externalize lodash.debounce explicitly, but bytemd has a lot more CJS-only deps that needs to be externalized. vite-plugin-svelte could use the same deps reinclusion logic from prebundling for here as well to automate this, but I think for dev only. And I'm not sure how safe it'll work in pnpm.

Or maybe adjustments can be made in Vite to externalized transitive deps by default.

@dominikg
Copy link
Member

replacing the unmaintained lodash.debounce with https://github.com/angus-c/just#just-debounce-it might be a quickfix for bytemd

@bluwy
Copy link
Member

bluwy commented Feb 27, 2022

There are a lot more deps there that has issues, like lodash.throttle, unified, and the remark plugins IIRC.

@dominikg
Copy link
Member

For lodash.throttle there is just-throttle too. but remark plugins don't always have an alternative that works with esm too i guess.

Looks like bytemd is actively working on their package structure, adding an exports field (see history from today) https://github.com/bytedance/bytemd/commits/main/packages/bytemd/package.json

@benmccann
Copy link
Member Author

I don't understand why we're only hitting this with this library. I thought there are lots of Svelte libraries with CJS deps that work.

@benmccann
Copy link
Member Author

benmccann commented Feb 27, 2022

I put a console.log statement at the end of resolveSSRExternal and found:

externals: ["@sveltejs/adapter-auto","@sveltejs/kit","svelte","@bytemd/plugin-gfm","bytemd > codemirror-ssr","bytemd > hast-util-sanitize","bytemd > lodash.debounce","bytemd > lodash.throttle","bytemd > rehype-raw","bytemd > rehype-sanitize","bytemd > rehype-stringify","bytemd > remark-parse","bytemd > remark-rehype","bytemd > unified","bytemd > unist-util-visit","bytemd > vfile","bytemd > word-count"]

But @bluwy said it wasn't being externalized, so I think perhaps something is going wrong in the Vite code in between where I was looking and where he was looking since I do see it being externalized

@bluwy
Copy link
Member

bluwy commented Feb 28, 2022

I don't understand why we're only hitting this with this library. I thought there are lots of Svelte libraries with CJS deps that work.

Me too. This looks like something we would have spotted long ago, maybe I'm missing something that's only an issue with bytemd specifically, but it looks to me that it's a general issue 🤔

I put a console.log statement at the end of resolveSSRExternal and found:

I tried the same and got [ '@sveltejs/adapter-auto', '@sveltejs/kit', 'svelte', '@bytemd/plugin-gfm' ] though. And I don't think the > syntax works with SSR externals. It would be a bug if there's a way to reproduce that.

I'll try to reproduce this in vite-plugin-svelte again tonight and see if it's a general issue not specific to bytemd

@benmccann
Copy link
Member Author

I tried the same and got [ '@sveltejs/adapter-auto', '@sveltejs/kit', 'svelte', '@bytemd/plugin-gfm' ] though. And I don't think the > syntax works with SSR externals. It would be a bug if there's a way to reproduce that.

Oh, I think that's because I accidentally had checked in experimental.prebundleSvelteLibraries: true. I just pushed a change to remove that (benmccann/sveltekit-bytemd-bug-reproduction@9d661d7). Hopefully you'll see the same as I do now

@bluwy
Copy link
Member

bluwy commented Feb 28, 2022

Ah yeah with experimental.prebundleSvelteLibraries: false I can see the same list now.

@benmccann
Copy link
Member Author

I don't think the > syntax works with SSR externals. It would be a bug if there's a way to reproduce that.

This seems to be the issue. It works (well the app is broken for other reasons, but reasons that make more sense) if I strip out the prefix:

    externals = externals.map(s => {
      const sep = ' > ';
      const i = s.lastIndexOf(sep);
      if (i < 0) {
        return s;
      }
      return s.substring(i + sep.length);
    });

Any idea where the code that inserts this might live or what a good solution to it might be?

@dominikg dominikg removed the triage Awaiting triage by a project member label Feb 28, 2022
@bluwy
Copy link
Member

bluwy commented Feb 28, 2022

I haven't got time to check this deeper today 😅 I think it's Vite that re-using the strings from optimizeDeps.include to ssr.external, hence the > there, but I can't seem to find where. If we can find it then I think it makes sense for Vite to support > in ssr.external too. Otherwise we can make the change in vite-plugin-svelte too by returning the correct externals here (may need some code to figure out the array, can be referenced here)

Re the code block in the comment, the syntax allows no spaces beside the > too, so ideally we substring from > onwards and trim it, which should be enough.

@benmccann
Copy link
Member Author

I think it's Vite that re-using the strings from optimizeDeps.include to ssr.external, hence the > there, but I can't seem to find where.

Yes, I think that's right. It looks like it's being passed as input to resolveSSRExternal. For example: https://github.com/vitejs/vite/blob/c703a3348adeaad9dc92d805a381866917f2a03b/packages/vite/src/node/server/index.ts#L371

I think it makes sense for Vite to support > in ssr.external too

Yeah, I agree that sounds like a good fix. Do you want to take a stab at that since you implemented it for optimizeDeps? I'm not sure I'd know how. The other option would be to strip everything before the > as I did above. If we want to go with that option I could clean it up and send a PR

@bluwy
Copy link
Member

bluwy commented Mar 2, 2022

Ah thanks for finding the source. I think a simple fix like the code you shown, and have it applied in the linked code would be enough. I’ve worked a bit on the fix in Vite today but having some hurdles making the tests. Feel free to make the patch too if you’d like, I haven’t put a lot of time into the fix yet.

@benmccann
Copy link
Member Author

Ok, I sent a PR here: vitejs/vite#7154

@bluwy
Copy link
Member

bluwy commented Mar 20, 2022

This should be fixed in vite-plugin-svelte 1.0.0-next.40 and Vite 2.9.0-beta.2

@bluwy bluwy closed this as completed Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants