Dependencies on node APIs

[pulsejet]

Can we please not depend on path? I would really like to get rid of node polyfills.

[susnux]

What is the issue you face? Because bundling those polyfills does not make much sense.
You will always need a bundler, so you can inject the polyfills your self, which will allow treeshaking and smaller bundle size as otherwise the same functions might get included multiple times just because they are bundled with each library.

[pulsejet]

Actually none of the other nextcloud packages (at least the ones I used) depend on node polyfills, which allowed me to get rid of NodePolyfillPlugin altogether. But then I had to add it back just for this package. If we use something like path-browserify instead that actually targets the browser then there's no need to change the downstream application's webpack config.

[pulsejet]

Also noteworthy: path depends on process so this also adds completely redundant code to the output bundle. The impact may be negligible though.

[pulsejet]

Hmm, the docs of path-browserify claim webpack injects it by default. I'm a bit confused now.

EDIT: I guess this was old documentation. We can still import from path-browserify directly and it seems to work fine

[susnux]

I am still not really convinced this makes sense, as the libraries are build for bundlers and thus you can simply use polyfills there. But especially because there are also some other dependencies that use node modules, and therefor if we bundle the polyfills you have to polyfill the dependencies by your self and thus get the same polyfill injected twice.

Meaning for this library if we include the path polyfills you still need the polyfills in your app config because this library depends on e.g. @nextcloud/files which makes heavy use of the path module.

Directly importing from browserify-path seems to be ok, but you still get that error from the dependencies of this library (and maybe others).

So I would prefer if we can agree on a common practice across @nextcloud/ libraries for handling node modules.

[pulsejet]

Meaning for this library if we include the path polyfills you still need the polyfills in your app config because this library depends on e.g. @nextcloud/files which makes heavy use of the path module.

Gotcha, didn't know we use it in files too.

So I would prefer if we can agree on a common practice across @nextcloud/ libraries for handling node modules.

Yeah this makes total sense. From my side, the reasons I think we should move away from node polyfilling:

1. This code is only ever supposed run in the browser, so polyfilling node functionailty is semantically not right. This code is never supposed to run in node, so it makes no sense to use node APIs.
2. It adds transitive dependencies that we don't need (e.g. process)
3. (most important) this causes the library to depend on the webpack configuration of the downstream plugin. Further, no idea what happens if the downstream app wants to switch to something other than webpack. The libraries should be as independent as possible.

Need input from more people on this. @skjnldsv any comments?

BTW the webdav client used to depend on node polyfills earlier, but that dependency too is gone in the latest version (they split node and browser modules).

[skjnldsv]

Need input from more people on this. @skjnldsv any comments?

Always up for cleaner approaches. Thought what library do you have in mind for the few path imports we're using (usually normalize, basename, extname, dirname and join)

[pulsejet]

browserify-path should work here, I believe. This is essentially the POSIX part of the node path module, which is the only thing needed in the browser. But the improvement is that it doesn't depend on process, plus no need for the extra polyfill plugin in the downstream webpack config.

[susnux]

Then how about https://www.npmjs.com/package/@nextcloud/paths ?