azure-sdk-for-js icon indicating copy to clipboard operation
azure-sdk-for-js copied to clipboard

shimming Node built-ins (Split @azure/storage-blob to browser and node versions)

Open koljada opened this issue 4 years ago • 11 comments

Is your feature request related to a problem? Please describe. Having a single package for node and browser APIs might cause some problems during development. For example I started using Vite instead of webpack and now I'm having following error when trying to run my project: [vite] Dep optimization failed with error: Dependency "@azure/storage-blob" is attempting to import Node built-in module "fs". This will not work in a browser environment.

Also with current design it's needed to constantly check the documentation in order to figure out whether some particular API method is supported in my environment. It kind of eliminates typescript and IDE benefits. Typically when you're using some package you would expect that all available API should be working.

Describe the solution you'd like There should be separate packages for browser and node versions.

Describe alternatives you've considered Vite build in prod mode is working but obviously it doesn't help during development(e.g. there is no hot module replacement)

Additional context I'm using:

  • "@azure/storage-blob": "^12.1.2"
  • "@azure/abort-controller": "^1.0.1"
  • "vite": "^1.0.0-beta.3"

koljada avatar Jun 24 '20 10:06 koljada

cc @xirzec, @jeremymeng for issues with vite cc @XiaoningLiu to respond to concern with different APIs in browser vs node

ramya-rao-a avatar Jun 24 '20 16:06 ramya-rao-a

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @xgithubtriage.

msftbot[bot] avatar Jun 24 '20 16:06 msftbot[bot]

Looks like the specific concerns are listed here: https://github.com/vitejs/vite/issues/450#issuecomment-648865571

xirzec avatar Jun 24 '20 18:06 xirzec

We do have this in package.json:

    "fs": false,
    "os": false,
    "process": false

Maybe Vite doesn't support this pattern?

xirzec avatar Jun 24 '20 18:06 xirzec

Personally I'd rather we move node-specific code into files we can map to browser equivalents, even if the browser equivalent is a no-op.

xirzec avatar Jun 24 '20 18:06 xirzec

We had the separation in storage track 1. However in track 1 nodejs/browser specific functions were module level functions. In track 2 design they are moved into clients to be instance functions. We couldn't find a good way to separate instance functions for example, put BlockBlobClient.uploadFile() into one file, put BlockBlobClient.uploadBrowser() into another file.

jeremymeng avatar Jun 24 '20 19:06 jeremymeng

We had the separation in storage track 1. However in track 1 nodejs/browser specific functions were module level functions. In track 2 design they are moved into clients to be instance functions. We couldn't find a good way to separate instance functions for example, put BlockBlobClient.uploadFile() into one file, put BlockBlobClient.uploadBrowser() into another file.

There are some ways (and I'm sure @richardpark-msft can teach you some composition magic with TS) but I think in the near term it would be satisfactory to simply move the parts of the code that import from a node module (e.g. fs) out into some kind of platform abstraction layer.

For example, a readFileFromPath helper method in a separate module that is replaced with a noop/throw implementation in the browser version. That way you're able to write import { readFileFromPath } from "./utils/fs" instead of doing a direct import.

xirzec avatar Jun 25 '20 04:06 xirzec

Good point! we already do that for fs.stat and should expand to cover other cases.

jeremymeng avatar Jun 25 '20 05:06 jeremymeng

Assign to @ljian3377 as CRI owner.

Agree. The ideal way is browser & node expose same APIs. SDK internally wraps the implementaion differenence. Actually, we already did this for many feature implementations, like download(), node & browser expose same resposne interface type but internally they have different implementation. We need to go through and check left cases mentionend by Jeremy.

I don't buy in the idea to have separate packages for node & browser. Once in storage track1, we have separate packages for node & browser. It's really a painful story and doesn't provide good user experiences. We have separate versioning, seprate documentations, separate packages, while 2 packages have some shared code and some not. Many track1 customers will come and ask why some API cannot work in browser, and actually they are mistakly using node version package.

XiaoningLiu avatar Jul 01 '20 06:07 XiaoningLiu

@xirzec @bterlson I think solving this by manually shimming Node built-ins in storage will be very inefficient. And this could be a very common need for all libraries that need to support both browser and Node.js. So could Azure SDK team help this out with some magic tool?

ljian3377 avatar Jul 01 '20 06:07 ljian3377

Partially fixed by https://github.com/Azure/azure-sdk-for-js/pull/9877.

ljian3377 avatar Aug 17 '20 02:08 ljian3377