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.

Sign up for GitHub

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

Jump to bottom

Update BlockBlobClient.uploadStream to use NodeJS.ReadableStream instead of Readable #11484

Closed
2 of 6 tasks
Ethan-Arrowood opened this issue Sep 25, 2020 · 5 comments
Closed
2 of 6 tasks

Update BlockBlobClient.uploadStream to use NodeJS.ReadableStream instead of Readable #11484

Ethan-Arrowood opened this issue Sep 25, 2020 · 5 comments
Assignees
@jiacfan
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. Docs needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Storage Storage Service (Queues, Blobs, Files)

Comments

@Ethan-Arrowood
Copy link

Ethan-Arrowood commented Sep 25, 2020 •
edited
Loading

  • Package Name: storage-blob
  • Package Version: 12
  • Operating system: mac
  • nodejs
    • version: 14
  • browser
    • name/version:
  • typescript
    • version: 4
  • Is the bug related to documentation in

Describe the bug
Due to this method using Readable type rather than NodeJS.ReadableStream type I cannot pass a standard Node.js readable stream. With this change, users can still pass Readable types to the method since it implements NodeJS.ReadableStream.

I previously tried to fix this with #11483 but didn't anticipate other methods require the Readable type. Maybe i misunderstand the difference between it and NodeJS.ReadableStream. In addition, maybe I need to do something in my own code.

In my specific instance I'm using Node.js and Busboy which defines its file type as NodeJS.ReadableStream (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/busboy/index.d.ts#L41)
@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 Sep 25, 2020
@Ethan-Arrowood
Copy link
Author

Did a bit more digging seems like I've been confused by new vs old streams using new Readable().wrap(oldStream) i was able to get everything working correctly and type safe. https://nodejs.org/api/stream.html#stream_readable_wrap_stream

I'll leave this open for others to comment. Maybe we can add something like this to the docs or examples?

@jeremymeng jeremymeng added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Sep 25, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 25, 2020
@jeremymeng jeremymeng added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Sep 25, 2020
@jiacfan
Copy link
Member

jiacfan commented Sep 25, 2020

Hey, @Ethan-Arrowood

Thanks for reaching us and good to know you have found the right approach. Yes, according to Readable's definition, using wrap is the right approach. This is a valuable topic related to old vs new stream, and might help others as well.

Thanks,
Jiachen

@XiaoningLiu XiaoningLiu assigned jiacfan and unassigned XiaoningLiu Sep 27, 2020
@ramya-rao-a
Copy link
Contributor

@jiacfan What are the next steps here? Are we looking at updating docs or something else?

@jiacfan jiacfan added the Docs label Oct 21, 2020
@jiacfan jiacfan mentioned this issue Oct 21, 2020
Closed
@jiacfan
Copy link
Member

jiacfan commented Oct 21, 2020 •
edited
Loading

This is part of Node.js usage which might be met when using old libraries by small groups of users. We tried to create a PR, and the comment added might add overhead for most common users, so temporarily we prefer to leave this as a Github issue. And will continuously tracking if there're more feedbacks coming, we can optimize the doc further if this becomes a common issue.

@jiacfan
Copy link
Member

jiacfan commented Oct 21, 2020

Close the issue as it's a node.js usage related pattern and welcome feedback if anyone hit similar isse.

@jiacfan jiacfan closed this as completed Oct 21, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this issue Nov 11, 2020
@paulgmiller @bcho
@levimm @mainred @norshtein @bowang-666
CodeGen from PR 11588 in Azure/azure-rest-api-specs
2b1b466 
Dev containerservice microsoft.container service 2020 11 01 (Azure#11588)

* Adds base for updating Microsoft.ContainerService from version stable/2020-09-01 to version 2020-11-01

* Updates readme

* Updates API version in new specs and examples

* add autoupgradeprofile to 2020-11-01 (Azure#11363)

* add autoupgradeprofile

* add autoupgrader profile to mc

* format change

Co-authored-by: Xiahe Liu <Xiahe.Liu@microsoft.com>

* add pod subnet id on agentpool (Azure#11310)

* aks: add pod identity profile spec (Azure#11366)

* add private dns zone property in 2020-11-01 (Azure#11311)

Co-authored-by: Li Ma <lima2@microsoft.com>

* Remove invalid pattern and adjust description (Azure#11312)

* aks: fix pod identity profile field names (Azure#11484)

* aks: add missing `podIdentityProfile.enabled` field

* aks: typo

* add non upgrade channel (Azure#11492)

Co-authored-by: Xiahe Liu <Xiahe.Liu@microsoft.com>

* Make identityProfile mutable (Azure#11458)

* Add KubeletConfig and LinuxOSConfig specs (Azure#11490)

* aks: update readme tag for 1101 (Azure#11472)

* aks: update readme tag

* aks: add `2020-11-01-only`

* resovle merge conlicts to master, adding recent master change to dev branch (Azure#11604)

Co-authored-by: Xiahe Liu <Xiahe.Liu@microsoft.com>

Co-authored-by: Xiahe Liu <Xiahe.Liu@microsoft.com>
Co-authored-by: Paul Miller <pmiller@microsoft.com>
Co-authored-by: hbc <me@hbc.rocks>
Co-authored-by: Super <mali_no2@hotmail.com>
Co-authored-by: Li Ma <lima2@microsoft.com>
Co-authored-by: Qingchuan Hao <haoqingchuan@hotmail.com>
Co-authored-by: Tongyao Si <tosi@microsoft.com>
Co-authored-by: Bo Wang <61758179+bowang-666@users.noreply.github.com>
Co-authored-by: Phoenix He <tih@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jiacfan jiacfan

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. Docs needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@XiaoningLiu @jeremymeng @jiacfan @Ethan-Arrowood @ramya-rao-a