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

vite build reports "fs" browser compatibility issues #988

Closed
Cmacu opened this issue Sep 28, 2024 · 11 comments · Fixed by #1144 or #1210
Closed

vite build reports "fs" browser compatibility issues #988

Cmacu opened this issue Sep 28, 2024 · 11 comments · Fixed by #1144 or #1210

Comments

@Cmacu
Copy link
Contributor

Cmacu commented Sep 28, 2024

When bundling for deployment I see the following warning in the console:

[plugin vite:resolve] Module "fs" has been externalized for browser compatibility, imported by "/node_modules/signaldb/dist/index.mjs". See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.
@maxnowack
Copy link
Owner

maxnowack commented Sep 30, 2024

I think this message is triggered by the createFilesystemAdapter. It shouldn't be a real issue.
It should go away when the persistence adapters are splitted in separate packages in v1

@TheeMachine
Copy link

  • Angular CLI: 17.3.6
  • Node: 20.17.0
  • Package Manager: npm 10.8.2

import { Collection, createLocalStorageAdapter } from 'signaldb';
import angularReactivityAdapter from 'signaldb-plugin-angular';

export const InventoryItems = new Collection<
  InventorySelectItem & { id: string }
>({
  reactivity: angularReactivityAdapter,
  persistence: createLocalStorageAdapter('localData'),
});

My code is above but I got this error and Angular app not build.

./node_modules/signaldb/dist/index.mjs:2954:25-37 - Error: Module not found: Error: Can't resolve 'fs' in '/project_path/node_modules/signaldb/dist'

Is there any solutions?

@maxnowack
Copy link
Owner

Maybe someone could create a small reproduction repo. I could try to find some workarounds until #992 is done and v1 was released

@ngryman
Copy link

ngryman commented Nov 20, 2024

For Vite users, this is not really an issue as the fs module will not be polyfilled by default.
To make the warning disappear, you can add it explicitly as an external dependency:

{
  // ...
  rollupOptions: {
    external: 'fs'
  }
}

@maxnowack Here's a minimal repro just in case: https://stackblitz.com/edit/signaldb-repro-fs-module.
I think a good solution would be to use the rollupOptions.output.preserveModules in your Vite config (ref: vitejs/vite#5174).
From what I understand it would make the output tree-shakable for other bundlers and you wouldn't need to do #992.

@maxnowack
Copy link
Owner

Thanks @ngryman! I think this is the configuration options I was looking for the whole time 😀
I think this could also resolve #1142.

The error doesn't pop up in your repro. However, it seems very plausible that this configuration change solves the issues and I currently don't see any real downside. I've changed it in #1144

@maxnowack
Copy link
Owner

Fix released with v0.23.0 🎉

@ngryman
Copy link

ngryman commented Nov 20, 2024

Wow that was fast 😀

Unfortunately I still have the issue with v0.23.0.

From my understanding, in order to be tree-shaked, functions needs to be pure (ie. no side-effects).
Bundlers seem to suck a bit at determining if a function is pure automatically, event with "sideEffects": false.

You can manually annotate pure function declarations with /*#__NO_SIDE_EFFECTS__*/ or @__NO_SIDE_EFFECTS__ when using JSDoc (rollup/rollup#5024). Annotated functions will be marked at safe to drop. A good start will probably to annotate: createFilesystemAdapter.

Another alternative is to mark the call sites of your functions with /* @__PURE */ but this tedious. There is a plugin that does automatically that for you: https://github.com/TomerAberbach/rollup-plugin-tree-shakeable. It's worth trying.

If only tooling was simpler 😅

@maxnowack
Copy link
Owner

Yeah, you're right tooling currently is a mess. Especially since the transition to ESM …
I got your repro running (I was expecting some error, but then realized that it's only a warning on build 😀). I've tried several things, like the annotations you mentioned and also turning off library mode and just use rollup like recommended here (https://stackoverflow.com/a/76184765).
No success so far. The message is still there 😕, even though it now has a different path.

[plugin:vite:resolve] [plugin vite:resolve] Module "fs" has been externalized for browser compatibility, imported by "node_modules/signaldb/dist/persistence/createFilesystemAdapter.mjs". See https://vite.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

@maxnowack maxnowack reopened this Nov 20, 2024
@ngryman
Copy link

ngryman commented Nov 20, 2024

Yes it's not really critical for Vite, except for the bundle size that could be improved for client-side usage with tree-shaking.

For other bundlers than Vite, it seems that it could produce an error, perhaps with Webpack? (cc @TheeMachine).

@maxpowernz
Copy link

tried using this?
https://www.npmjs.com/package/vite-plugin-external

@ngryman
Copy link

ngryman commented Nov 25, 2024

@maxpowernz Vite already marks Node.js dependencies as external by default, so no polyfill will end up in the final bundle. The problem may still be present for other bundlers though (eg. Webpack). I don't know if they behave the same way Vite does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants