Stop using node modules in client-side libraries

[alabaki]

Update 2022-10-12

We've addressed the url, assert, and events issues. However, buffer still needs to be polyfilled in some cases. Still investigating that.

Describe the bug

Currently, the azure-client package and its dependencies driver-utils and container-loader, are using node-only builtins when it's intended to be a client-side library, therefore it is currently impossible to use this library with Vite or Webpack 5.

The node builtin that are being used are: url and querystring

To Reproduce

1. Start a client-side project using Vite as a bundler
2. Add the azure-client package
3. Build

The build will fail with errors:

Expected behavior

Successful build and use

Tracking issues

• Review use of node APIs #9506

[tylerbutler]

@alabaki Do you have a project reproducing this that you can share?

[alabaki]

@tylerbutler Please refer to:
https://github.com/alabaki/azure-client-issue

npm install
npm run dev

[tylerbutler]

@alabaki Thank you! I was able to get past the node dependencies by including the polyfills. My fork of your repo is here if you want to see those changes. I'll use this issue to track getting the full list of node deps and either removing them or replacing them with isomorphic code.

Anyway, the polyfills will unblock the build, but there is still a problem related to the debug package.

'debug' is not exported by node_modules\debug\src\browser.js, imported by node_modules\@fluidframework\telemetry-utils\lib\debugLogger.js
file: E:/code/azure-client-issue/node_modules/@fluidframework/telemetry-utils/lib/debugLogger.js:6:9
4:  */
5: import { performance } from "@fluidframework/common-utils";
6: import { debug as registerDebug } from "debug";
            ^
7: import { TelemetryLogger, MultiSinkLogger, ChildLogger } from "./logger";
8: /**
error during build:
Error: 'debug' is not exported by node_modules\debug\src\browser.js, imported by node_modules\@fluidframework\telemetry-utils\lib\debugLogger.js
    at error (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:158:30)
    at Module.error (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:12423:16)
    at Module.traceVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:12808:29)
    at ModuleScope.findVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:11588:39)
    at ChildScope.findVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:6953:38)
    at ClassBodyScope.findVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:6953:38)
    at FunctionScope.findVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:6953:38)
    at ChildScope.findVariable (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:6953:38)
    at Identifier.bind (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:6468:40)
    at CallExpression.bind (E:\code\azure-client-issue\node_modules\rollup\dist\shared\rollup.js:5076:23)

I haven't figured out that issue yet.

[tylerbutler]

Related issues:

#8298
#4843
#6623

[alabaki]

@tylerbutler Thank you! I believe you didn't push your changes to the forked repo, however, I also replaced the url module with a polyfill and it solved that issue. Regarding the debug package issue, I included the @types/debug package and now it's building properly, you can refer to the repo to see the changes.

But just like you mentioned, all node modules need to be replaced. Thanks for the help!

[tylerbutler]

@alabaki You're right, I forgot to push to my fork. 🤦🏻‍♂️ Thank you for sharing the changes you made to get unblocked!

[malekpour]

There are more node dependencies beyond version 0.54. If you upgrade client dependencies, the repo doesn't works anymore.

Since 0.55, @fluidframework/routerlicious-driver depends on node-fetch which requires stream, http, process, buffer, util and I was not able to get it to work with vite even after providing polypills.

9d1fcb2

[heliocliu]

There are more node dependencies beyond version 0.54. If you upgrade client dependencies, the repo doesn't works anymore.

Since 0.55, @fluidframework/routerlicious-driver depends on node-fetch which requires stream, http, process, buffer, util and I was not able to get it to work with vite even after providing polypills.

9d1fcb2

oof
@znewton Can you comment on the intent of the original change? It looks like node-fetch is targeted for node environments but routerlicious-driver needs to be browser-compatible. The webpack issues described above make these errors less apparent as part of our validation unfortunately.

[znewton]

Yes absolutely. The reason for switching to fetch was that Axios doesn't provide streaming capability which was something we wanted to experiment with. Unfortunately, the choice to use node-fetch was simply "ODSP driver established a precedent of using node-fetch in browser and Node, so it should be fine." That, plus the fact (unknown to me) that Webpack 4 automatically bundles Node polyfills made me think that there was no problem.

I'm actively working on righting this by switching to cross-fetch since @tylerbutler brought this to my attention this morning, but am sadly hung up on mocking it for UTs.

[curtisman]

Just FYI, we currently use node-fetch v2, which just bind to the browser version of fetch in the browser context and should not require the node polyfills.

node-fetch v3 dropped supported for browser support (see link)

So if we upgrade to it, we will have to put in a shim ourselves or switch to other libraires.

[malekpour]

Fetch API has landed into Node.js since v17.5 and will be a first class citizen in Node.js 18 (LTS). Probably it makes sense to call global fetch() in both node and browser instead of referencing node-fetch directly. As per suggestion by the node-fetch authors, you can call global fetch() in node using the following method.

Main benefits:

• Allows to switch to node-fetch v3
• Uses native browser implementation
• Uses native node implementation when available

// fetch-polyfill.js
import fetch, {
  Blob,
  blobFrom,
  blobFromSync,
  File,
  fileFrom,
  fileFromSync,
  FormData,
  Headers,
  Request,
  Response,
} from 'node-fetch'

if (!globalThis.fetch) {
  globalThis.fetch = fetch
  globalThis.Headers = Headers
  globalThis.Request = Request
  globalThis.Response = Response
}

// index.js
import './fetch-polyfill'

[malekpour]

@znewton, wasn't node-fetch v3 a better option?

[znewton]

@malekpour Upgrading to node-fetch v3 would impact everywhere else in the repo that uses node-fetch. If what @curtisman says is true, then our use of node-fetch v2 is important for other places (e.g. odsp-driver) to not require Node.js polyfills. I did not want to take on the scope of altering their fetch usage.

[curtisman]

Upgrading to node-fetch v3 is possible as long as we add the shim to redirect it to the browser implementation ourselves.

I haven't look into cross-fetch, but I believe it has browser polyfill as well? My concern would be that it will increase bundle size in the browser context because of the unused polyfill (we don't support browser that doesn't have the 'fetch' API) Would be good to make sure that cross-fetch doesn't bring additional code for browser.

[malekpour]

I think cross-fetch is using node-fetch v2 for node and WhatWG-Fetch for browser.

[znewton]

According to cross-fetch README,

Just like isomorphic-fetch, it is just a proxy. If you're in node, it delivers you the node-fetch library, if you're in a browser or React Native, it delivers you the github's whatwg-fetch. The same strategy applies whether you're using polyfill or ponyfill.

So I guess it does indeed add the polyfill/ponyfill of WhatWG-Fetch :/

My main hesitation for touching ODSP's node-fetch usage is I don't know how to locally validate odsp-driver changes. @vladsud thoughts here?

[znewton]

In the section of the node-fetch upgrade guide that @curtisman linked to, they seem to recommend using cross-fetch "if you are using node-fetch client-side". Switching the r11s-driver to cross-fetch would immediately solve this issue, but would indeed increase bundle size as Curtis pointed out.

We have a separately versioned package, common-utils that already has a split browser/node entrypoint. Would it make sense to more slowly resolve this issue by exposing fetch from common-utils via adding fetchNode.ts (using node-fetch v3) and fetchBrowser.ts (relying on built-in fetch) there? Then when that version of common-utils is released we can consume it within R11s-driver and ODSP-driver at our teams' individual paces.

[znewton]

Merged #9433 to resolve the immediate issue that r11s-driver does not work without host adding their own Node polyfills. Opened #9443 to track improvement of the solution so that we don't include Fetch polyfills (i.e. WhatWG-Fetch).

[AndrewCraswell]

I ran into this issue 6 months ago when I was trying to onboard Fluid Framework for our app. We also use Vite, and I ended up giving up feeling like the Client libraries aren't ready to play around with yet. I started playing around with it again this weekend and am disappointed to see this is still an issue.

The workarounds with polyfills described above also weren't working for me anymore. I had to install the url Node package into my project directly to resolve the build issues.

[alabaki]

@tylerbutler What is the update here? Will the packages be fixed? Most front-end frameworks are now using webpack 5 or vite rendering this library unusable

[taylorsw04]

@curtisman @tylerbutler bumping this thread.

[tylerbutler]

@RishhiB is actively working on several issues related to this. 👍

[curtisman]

@RishhiB do you have ADO item tracking this? What's the ETA?

[tylerbutler]

We've addressed the url, assert, and events issues, and these changes will be in the next release of azure-client and related packages. However, buffer still needs to be polyfilled in some cases. Still investigating that.

[microsoft-github-policy-service]

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!