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

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

Closed
koljada opened this issue Jun 24, 2020 · 12 comments
Closed
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Storage Storage Service (Queues, Blobs, Files)

Comments

@koljada
Copy link

koljada commented Jun 24, 2020

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"
@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jun 24, 2020
@ramya-rao-a
Copy link
Contributor

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

@ramya-rao-a ramya-rao-a added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jun 24, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 24, 2020
@ghost
Copy link

ghost commented Jun 24, 2020

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

@xirzec
Copy link
Member

xirzec commented Jun 24, 2020

Looks like the specific concerns are listed here: vitejs/vite#450 (comment)

@xirzec
Copy link
Member

xirzec commented Jun 24, 2020

We do have this in package.json:

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

Maybe Vite doesn't support this pattern?

@xirzec
Copy link
Member

xirzec commented Jun 24, 2020

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.

@jeremymeng
Copy link
Member

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.

@xirzec
Copy link
Member

xirzec commented Jun 25, 2020

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.

@jeremymeng
Copy link
Member

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

@XiaoningLiu
Copy link
Member

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.

@ljian3377
Copy link
Member

ljian3377 commented Jul 1, 2020

@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?

@XiaoningLiu XiaoningLiu removed their assignment Jul 15, 2020
@ljian3377 ljian3377 added enhancement and removed question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jul 15, 2020
@ramya-rao-a ramya-rao-a added feature-request This issue requires a new behavior in the product in order be resolved. and removed enhancement labels Jul 20, 2020
@ramya-rao-a ramya-rao-a added this to the Backlog milestone Jul 20, 2020
@ljian3377
Copy link
Member

Partially fixed by #9877.

@ljian3377 ljian3377 changed the title Split @azure/storage-blob to browser and node versions shimming Node built-ins (Split @azure/storage-blob to browser and node versions) Sep 10, 2020
@ljian3377 ljian3377 removed this from the Backlog milestone Nov 3, 2020
@ljian3377 ljian3377 added this to the [2021] June milestone Nov 3, 2020
@EmmaZhu EmmaZhu modified the milestones: [2021] June, [2021] August Jul 9, 2021
@xirzec xirzec modified the milestones: [2022] February, [2022] June May 17, 2022
@xirzec xirzec modified the milestones: [2022] June, [2022] July Jun 13, 2022
@xirzec xirzec modified the milestones: 2022-07, 2022-08 Jul 8, 2022
@xirzec xirzec modified the milestones: 2022-08, 2022-09 Aug 9, 2022
@xirzec xirzec modified the milestones: 2022-09, 2022-12 Nov 1, 2022
@xirzec xirzec modified the milestones: 2022-12, 2023-02 Jan 11, 2023
@xirzec xirzec removed this from the 2023-02 milestone Mar 31, 2023
Copy link

Hi @koljada, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 20, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. feature-request This issue requires a new behavior in the product in order be resolved. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests

7 participants