[storage blob] Change from isNode to isNodeCompatible #29083
Conversation
mpodwysocki
requested review from
bterlson,
xirzec,
jeremymeng,
EmmaZhu,
XiaoningLiu,
vinjiang,
jiacfan,
ljian3377,
deyaaeldeen and
timovv
as code owners
github-actions
bot
added
Azure.Core
Storage
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
There was a problem hiding this comment.
Makes sense overall, although we should probably bump the min-version for storage-blob's core-util dependency to ensure this constant exists.
Edit to add: which reminds me, is there anything in our release pipeline stopping storage from accidentally releasing with a dependency on a core version that has not been released yet? If not, do we need to release core-util before making storage-blob changes?
| @@ -74,6 +74,11 @@ export const isNode = | |||
| !isDeno && | |||
There was a problem hiding this comment.
GH did not let me comment over the right line (69) but I wonder if we should mark isNode as @deprecated so package owners know to use isNodeCompatible instead
There was a problem hiding this comment.
That's possible, @xirzec any thoughts?
There was a problem hiding this comment.
Feels a bit odd to me having isDeno and isBun around while deprecating isNode. I wonder if we will ever run into a scenario in future where we will be wanting to check for Node specifically, intentionally excluding Deno/Bun?
There was a problem hiding this comment.
Yeah I think we should deprecate the existing one if we're reverting the behavior to match isNodeCompatible (really just exporting it twice with different names.)
If we think there's a case for "is actually node" then perhaps we can use a worse name for it? like isNodeRuntime ? And then clarify in the doc comments all the options
There was a problem hiding this comment.
Oh if we're going to change the behavior of isNode to be the same as the new flag then I support deprecating but that doesn't seem to be what we're doing in this PR?
mpodwysocki
force-pushed
the
feat/node-compatible
branch
from
ca85302 to
14377bf
Compare
| @@ -12,7 +12,7 @@ import { | |||
| PipelineResponse, | |||
| SendRequest, | |||
| } from "@azure/core-rest-pipeline"; | |||
| import { isNode } from "@azure/core-util"; | |||
| import { isNodeLike } from "@azure/core-util"; | |||
There was a problem hiding this comment.
do we really need to adopt this right away? I think if we're reverting the behavior in core-util we should do the migration off the deprecated check in its own PR and we can unblock customers by shipping core-util with the fixed behavior
### Packages impacted by this PR - @azure/core-util ### Issues associated with this PR - #28101 ### Describe the problem that is addressed by this PR Reverts the behavior of `isNode` checks to ensure that it can still return true for Bun and Deno. Introducing `isNodeLike` to also have the same behavior as `isNode` so we can deprecate the former function. We have also added `isNodeRuntime` which then checks for an explicit Node.js runtime. ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? The previous attempt was to create a `isNodeLike` and be opt-in, versus this PR which gives the old behavior and new opt-in for strict Node runtimes. ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ - #29083 ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [x] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Deyaaeldeen Almahallawi <[email protected]>
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
By restricting
isNodeto ensure strict Node compatibility and to exclude Bun and Deno, we have broken existing customers using the Node/npm compat features. This introduces theisNodeCompatibleboolean which allows us to determine whether it is either Node, Bun, or Deno through its Node/npm compat.What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
We could have undone the
isNodeto allow for Deno and Bun, but introducing a third option seemed better not to create any unexpected behavior when Node and only Node is expected. We should migrate as much as possible to the new approach.Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists