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

[core][client] Stop importing URL from "url" #15933

Closed
4 tasks
xirzec opened this issue Jun 23, 2021 · 3 comments
Closed
4 tasks

[core][client] Stop importing URL from "url" #15933

xirzec opened this issue Jun 23, 2021 · 3 comments
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. good first issue This issue tracks work that may be a good starting point for a first-time contributor help wanted This issue is tracking work for which community contributions would be welcomed and appreciated

Comments

@xirzec
Copy link
Member

xirzec commented Jun 23, 2021

Now that we're not trying for node 8 compatibility, we can simply use URL and URLSearchParams directly, eliminating the need for a browser and node version of this import.

We'll have to update our @types/node beyond 8 to make this happen, though! This already done in #15996 -- special thanks to @ramya-rao-a )

To fix this:

  • Set up your dev environment
  • Find instances where we import from the "url" module (grep for from "url")
  • Remove any shim files that do this import (e.g. url.ts and url.browser.ts) and clean up modules that import them. Now that URL and URLSearchParams are globally available, you can just remove the imports and the global version will start working.
  • Update the package.json for any SDK where you removed a .browser. file to remove the mapping in the browser section.
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 23, 2021
@ramya-rao-a ramya-rao-a added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Jun 24, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 24, 2021
@ramya-rao-a ramya-rao-a added this to the Backlog milestone Jun 24, 2021
@xirzec xirzec added good first issue This issue tracks work that may be a good starting point for a first-time contributor help wanted This issue is tracking work for which community contributions would be welcomed and appreciated labels Jul 26, 2021
@SuyashSonawane
Copy link
Contributor

Hi @xirzec, is this still open, I would like to give this a try.

@SuyashSonawane
Copy link
Contributor

I started working on it just to check how it can be done
Steps followed

  1. Set up environment
  2. created branch named stop-importing-from-url
  3. Found instances of from 'url' in core-client API
  4. Removed imports and files url.ts, url.browser.ts
  5. Updated package.json by removing url.browser.js entry
  6. Ran rushx test:node and successfully passed all tests (trace below)

Found configuration in /home/suyash/open-source/azure-sdk-for-js/rush.json

Rush Multi-Project Build Tool 5.51.1 - https://rushjs.io
Node.js version is 10.19.0 (LTS)

Found configuration in /home/suyash/open-source/azure-sdk-for-js/rush.json

Executing: "npm run clean && tsc -p . && npm run unit-test:node && npm run integration-test:node"

@azure/core-client@1.3.1 clean /home/suyash/open-source/azure-sdk-for-js/sdk/core/core-client
rimraf dist dist-* temp types *.tgz *.log

@azure/core-client@1.3.1 unit-test:node /home/suyash/open-source/azure-sdk-for-js/sdk/core/core-client
cross-env TS_NODE_FILES=true mocha -r esm -r ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 50000 --full-trace --exclude "test//browser/*.spec.ts" "test//*.spec.ts"

deserializationPolicy
✓ should not modify a request that has no request body mapper
✓ should parse a JSON response body
✓ should parse a JSON response body with a charset specified in Content-Type
✓ should parse a JSON response body with an uppercase Content-Type
✓ should parse a JSON response body with a missing Content-Type
parse(HttpOperationResponse)
✓ with no response headers or body
✓ with xml response body, application/xml content-type, but no operation spec
✓ with xml response body with child element with attributes and value, application/xml content-type, but no operation spec
✓ with xml response body, application/xml content-type, and operation spec for only String value
✓ with xml response body, application/xml content-type, and operation spec for only number value
✓ with xml response body, application/xml content-type, and operation spec for only headers
✓ with xml response body, application/atom+xml content-type, but no operation spec
✓ with xml property with attribute and value, application/atom+xml content-type, but no operation spec
✓ with xml property with attribute and value, my/weird-xml content-type, but no operation spec
✓ with service bus response body, application/atom+xml content-type, and no operationSpec
✓ should deserialize underscore xml element with custom xml char key
✓ with custom xml char key
✓ with default response headers
✓ with non default error response headers
✓ should throw when the response code is not defined in the operationSpec
✓ with non default complex error response
✓ with default error response body

serializationPolicy
serializeRequestBody()
✓ should serialize additional properties when the mapper is refd by className
✓ should serialize a JSON false request body
✓ should serialize a JSON null request body
✓ should serialize a JSON String request body
✓ should serialize a JSON String request body with namespace, ignoring namespace
✓ should serialize a JSON ByteArray request body
✓ should serialize a JSON ByteArray request body, ignoring xml properties
✓ should serialize a JSON Stream request body
✓ should serialize a JSON Stream request body
✓ should serialize an XML String request body
✓ should serialize an XML String request body, with namespace
✓ should serialize an XML ByteArray request body
✓ should serialize an XML Stream request body
✓ should serialize an XML Stream request body, with namespace
✓ should serialize an XML ByteArray request body with namespace and prefix
✓ should serialize an XML Composite request body
✓ should serialize a JSON Composite request body, ignoring XML metadata
✓ should serialize an XML Array request body with namespace and prefix
✓ should serialize a JSON Array request body, ignoring XML metadata
✓ should serialize an XML Array of composite elements, namespace and prefix
✓ should serialize an XML Composite request body with namespace and prefix
✓ should serialize an XML Dictionary request body
✓ should serialize an XML Dictionary request body, with namespace and prefix
✓ should serialize a JSON Dictionary request body, ignoring xml metadata
✓ should serialize an XML request body with custom xml char key
✓ should serialize a string send to a text/plain endpoint as just a string
✓ should serialize a string send with the mediaType 'text' as just a string
serializeHeaders()
✓ should respect customHeaders

Serializer
serialize
✓ should correctly serialize flattened properties
✓ should correctly serialize flattened properties when flattened constant is defined first
✓ should correctly serialize a string if the type is 'any'
✓ should correctly serialize an array if the type is 'any'
✓ should correctly serialize a string
✓ should correctly serialize a uuid
✓ should throw an error if the value is not a valid Uuid
✓ should correctly serialize a number
✓ should correctly serialize a boolean
✓ should correctly serialize an Enum
✓ should throw an error if the value is not valid for an Enum
✓ should correctly serialize a ByteArray Object
✓ should correctly serialize a Date Object
✓ should correctly serialize a Date object with max value
✓ should correctly serialize a Date object with max value and format UnixTime
✓ should correctly serialize a string in DateTimeRfc1123
✓ should correctly serialize an ISO 8601 duration
✓ should throw an error when given an invalid ISO 8601 duration
✓ should correctly serialize an array of primitives
✓ should correctly serialize an array of array of primitives
✓ should correctly serialize an array of array of object types
✓ should fail while serializing an array of array of "object" types when a null value is provided
✓ should correctly serialize an array of dictionary of primitives
✓ should correctly serialize a dictionary of primitives
✓ should correctly serialize a dictionary of array of primitives
✓ should correctly serialize a dictionary of dictionary of primitives
✓ should correctly serialize a composite type
✓ should correctly serialize object version of polymorphic discriminator
✓ should correctly serialize additionalProperties when the mapper knows that additional properties are allowed
✓ should allow null when required: true and nullable: true
✓ should not allow undefined when required: true and nullable: true
✓ should not allow null when required: true and nullable: false
✓ should not allow undefined when required: true and nullable: false
✓ should not allow null when required: true and nullable is undefined
✓ should not allow undefined when required: true and nullable is undefined
✓ should allow null when required: false and nullable: true
✓ should not allow null when required: false and nullable: false
✓ should allow null when required: false and nullable is undefined
✓ should allow undefined when required: false and nullable: true
✓ should allow undefined when required: false and nullable: false
✓ should allow undefined when required: false and nullable is undefined
deserialize
✓ should correctly deserialize a Date if the type is 'any'
✓ should correctly deserialize an array if the type is 'any'
✓ should correctly deserialize a uuid
✓ should correctly deserialize a composite type
✓ should correctly deserialize a pageable type without nextLink
✓ should correctly deserialize a pageable type with nextLink first in mapper
✓ should correctly deserialize a pageable type with nextLink
✓ should correctly deserialize object version of polymorphic discriminator
✓ should correctly deserialize an array of array of object types
✓ should correctly deserialize without failing when encountering unrecognized discriminator
✓ should correctly deserialize additionalProperties when the mapper knows that additional properties are allowed
✓ should deserialize headerCollectionPrefix
composite type
✓ should be deserialized properly when polymorphicDiscriminator specified
✓ should be deserialized properly when polymorphicDiscriminator specified in nested property
✓ should be deserialized properly when polymorphicDiscriminator specified in the parent
✓ should be deserialized properly when responseBody is an empty string
✓ should be deserialized properly when item list wrapper is an empty string
polymorphic composite type array
✓ should be deserialized with child properties
✓ should be serialized with child properties

ServiceClient
✓ should serialize headerCollectionPrefix
✓ should call rawResponseCallback with the full response
✓ should serialize collection:csv query parameters
✓ should serialize collection:csv query parameters with commas & skipEncoding true
✓ should serialize collection:csv query parameters with commas
✓ should serialize collection:csv query parameters with undefined and null
✓ should serialize collection:tsv query parameters with undefined and null
✓ should serialize collection:ssv query parameters with undefined and null
✓ should serialize collection:multi query parameters
✓ should deserialize response bodies
✓ should deserialize array response as null if it is empty and nullable
✓ should deserialize array response as empty array if it is empty and not nullable
✓ should deserialize dictionary response as null if it is empty and nullable
✓ should deserialize dictionary response as empty if it is empty and not nullable
✓ should deserialize object response as null if it is empty and nullable
✓ should deserialize object response as empty if it is empty and not nullable
✓ should deserialize primitive response as null if it is empty and nullable
✓ should deserialize primitive response as undefined if it is empty and not nullable
✓ should deserialize a head request with no body even if the mapper says the body is nullable
✓ should deserialize error response headers
✓ should deserialize non-streaming default response
✓ should re-use the common instance of DefaultHttpClient
✓ should not allow insecure connection by default
✓ should allow insecure connection if configured via client options
✓ should allow insecure connection if configured via request options
✓ should wrap body when bodyWrapper is specified
✓ should catch the mandatory parameter missing error
✓ should catch the mandatory parameter missing error in the query
Auth scopes
✓ should throw if scopes contain an invalid url
✓ should throw is no scope or baseUri are defined
✓ should use baseUrl to build scope
✓ should use the provided scope
getOperationArgumentValueFromParameter()
✓ should return undefined when the parameter path isn't found in the operation arguments or service client
✓ should return undefined when the parameter path is found in the operation arguments but is undefined and doesn't have a default value
✓ should return null when the parameter path is null in the operation arguments but is undefined and doesn't have a default value
✓ should return the operation argument value when the parameter path is found in the operation arguments
✓ should return the service client value when the parameter path is found in the service client
✓ should return the operation argument value when the parameter path is found in both the operation arguments and the service client
✓ should return the default value when it is a constant and the parameter path doesn't exist in other places
✓ should return the default value when it is a constant and the parameter path exists in other places
✓ should return undefined when the parameter path isn't found in the operation arguments or the service client, the parameter is optional, and it has a default value
✓ should return the default value when the parameter path isn't found in the operation arguments or the service client, the parameter is required, and it has a default value
✓ should return the default value when the parameter path is partially found in the operation arguments, the parameter is required, and it has a default value
✓ should return undefined when the parameter path is partially found in the operation arguments, the parameter is optional, and it has a default value
✓ should return the default value when the parameter path is not found in the options operation arguments and it has a default value
✓ should return undefined when the parameter path is not found as a property on a parameter within the options operation arguments and it has a default value
✓ should return null when the parameter path is null in the operation arguments but is undefined and it has a default value
✓ should return the service client value when the parameter path is found in the service client and it has a default value

getRequestUrl
✓ should handle nested replacements
✓ should handle query parameters on the base url
✓ should not modify needlessly encoded query parameters
✓ should allow empty query parameter value
✓ should work with replacement having both path and search part
✓ should create url when there is no existing value
✓ should create url when the existing value is an array and the new value is not an array
✓ should create url when the existing value is an array and the new value is also the same array
✓ should create url when the existing value is an array and the new value is a different array
✓ should create url when the existing value is not an array and the new value is the same value
✓ should create url when the existing value is not an array and the new value is a different value
✓ should create url when the existing value is not an array and the new value is an array

170 passing (201ms)

@azure/core-client@1.3.1 integration-test:node /home/suyash/open-source/azure-sdk-for-js/sdk/core/core-client
echo skipped

skipped

@xirzec
Copy link
Member Author

xirzec commented Aug 23, 2021

Fixed by #17022

@xirzec xirzec closed this as completed Aug 23, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this issue Sep 14, 2021
Adding identityBasedRestoreDetails optional property for the customers to be able to specify target storage account id when wanting to do restore using managed identities. (Azure#15933)
@xirzec xirzec removed this from the Backlog milestone May 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. good first issue This issue tracks work that may be a good starting point for a first-time contributor help wanted This issue is tracking work for which community contributions would be welcomed and appreciated
Projects
None yet
Development

No branches or pull requests

3 participants