Deprecating older packages by joheredi · Pull Request #19897 · Azure/azure-sdk-for-js

joheredi · 2022-01-18T20:05:31Z

Checklists

Added impacted package name to the issue description

Packages impacted by this PR:

@azure-rest/purview-account - Deprecated in favor of @azure-rest/purview-administration which includes purview account
@azure-rest/core-client-paging - Deprecated in favor of @azure/core-paging
@azure-rest/core-client-lro - Deprecated in favor of @azure/core-lro
@azure-rest/agrifood-farming - Re-generated using the latest codegen version and swagger, which updates paging and lro dependencies.

Issues associated with this PR:
#18729

Describe the problem that is addressed by this PR:
There are a few packages that need to be drprecated. This PR drops any dependencies on those packages and removes code from the repository to prepare for deprecation. After this PR is merged NPM packages will be marked as deprecated

Command used to generate this PR:(Applicable only to SDK release request PRs)
rushx generate:client for agrifood-farming

xirzec

I really like that we are able to re-use the same core packages for REST now.

I think it would have felt a little better to do the FarmBeats change in a separate PR from the removals, especially since its API surface is unreviewably large, but I didn't think of that until I scrolled through most of it. I left a few comments :)

xirzec · 2022-01-18T21:08:54Z

      "versionPolicyName": "core"
    },
-    {
-      "packageName": "@azure-rest/core-client-lro",


were any of these in the vs code workspace file?

xirzec · 2022-01-18T21:09:52Z

    area?: Measure;
    associatedBoundaryId?: string;
    attachmentsLink?: string;
    avgMaterial?: Measure;
-    createdDateTime?: Date;
+    createdDateTime?: Date | string;


this feels strange to me. Presumably if it's a string, it's something we can parse into a Date, so why make the consumer have to handle both?

xirzec · 2022-01-18T21:11:14Z

+    operationEndDateTime?: Date | string;
+    operationModifiedDateTime?: Date | string;
+    operationStartDateTime?: Date | string;
+    properties?: Record<string, Record<string, unknown>>;


So it's a property bag of property bags? The previous type was just Record<string, unknown> so why the additional nesting?

xirzec · 2022-01-18T21:12:17Z

+    Description?: string;
+    ETag?: string;
+    FarmerId?: string;
+    file?: string | Uint8Array;


wonder if this should eventually support streams...

Seems like these are form-encoded in the request, so maybe we have to collect it into a buffer anyway before sending? I'm interested in whether that can be supported without core support for doing it.

Yeah we need to support streams. I'm planning to pick up again the RLC work to support streams now that we have the fetch client available.

Right now I believe we'd just be able to send the file if it is loaded in memory as a Uint8Array or string, which of course is problematic for larg files.

I think I'll take on this for the March milestone

xirzec · 2022-01-18T21:15:24Z

+   * 
+###### Note:
+   * 
+1. The **‘contentType’** in the request header should be **'application/merge-patch+json'**.


it feels strange that we are doing **'contentType'** - especially since the single quotes look like smart quotes?

Also the header is "Content-Type" not "contentType"... but I am guessing this remark comes from swagger?

Yeah this is coming from the Swagger

azure-sdk · 2022-01-19T00:32:14Z

API changes have been detected in @azure-rest/agrifood-farming. You can review API changes here

witemple-msft

Couple of comments I noticed passing through.

witemple-msft · 2022-01-19T21:49:25Z

+    names?: Array<string>;
+    operationBoundaryIds?: Array<string>;
+    propertyFilters?: Array<string>;
+    sources?: Array<string>;


I have to wonder why all the X[] types changed to Array<X>. Most style guides I've read prefer the former.

Thanks Will, Is there any difference for Typescript when using Array<X> vs X[]?

I have filed an issue to track RLC design questions and added this one. Azure/autorest.typescript#1286

No, there is no type-level difference, just a stylistic one.

witemple-msft · 2022-01-19T21:50:15Z

-    minOperationEndDateTime?: Date;
-    minOperationModifiedDateTime?: Date;
-    minOperationStartDateTime?: Date;
+    minCreatedDateTime?: Date | string;


Feels pretty bad to have to constantly check to see if a value is a Date instance or a date-formatted string.

Hi Will, we are generating this union for Input types only, Output is always a string because we cannot safely deserialize without runtime mapping*.

(*note that this is an RLC which don't have any runtime mapping)

The reason these inputs are string or Date is to give the user a chance to work with them as it is more convenient for their use case.

For example, if they are round-tripping data from and to the service it may be more convenient to just take the date as received from the service and put it in the request without having to convert it to a Date. On the other hand, if users are crafting a request from scratch, it may be more convenient to say Date.now() than crafting the date as a string.

I believe this has value, especially since it is for input types, users shouldn't need flow control to work with these types.

it is for input types

I didn't catch this! If it's for inputs but not outputs then it doesn't feel bad anymore.

I also didn't see that it was only for input types 😅

That's a cool feature though and I like it alot!

witemple-msft · 2022-01-19T21:53:47Z

+    Description?: string;
+    ETag?: string;
+    FarmerId?: string;
+    file?: string | Uint8Array;


Seems like these are form-encoded in the request, so maybe we have to collect it into a buffer anyway before sending? I'm interested in whether that can be supported without core support for doing it.

witemple-msft · 2022-01-19T21:55:21Z

    name?: string;
-    properties?: FarmerPropertiesDictionary;


I don't see any other instances of this type name other than its deletion here. Did we mean to change these from named property bags to anonymous maps to any?

joheredi · 2022-01-25T20:19:22Z

Abandoning this PR to split it in 2. The first one #20059 will update Agrifood with the latest code gen version and remove dependency in the packages about to be deprecated. Following a PR to delete the deprecated code

**Checklists** - [x] Added impacted package name to the issue description **Packages impacted by this PR:** @azure-rest/agrifood-farming **Describe the problem that is addressed by this PR:** Re-generate Agrifood Farming using the latest version of RLC generator. This package was one of the first-ever RLCs generated and codegen has changes quite a bit since then. Regenerating against the same swagger. Major changes since last generation: - Switched away from @azure-rest/core-client-lro and @azure-rest/core-client-paging in favor of @azure/core-lro and @azure/core-client - Split Output and Input models **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)_ NO **Are there test cases added in this PR?**_(If not, why?)_ NO, it already has tests **Provide a list of related PRs**_(if any)_ #19897 **Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ `rushx generate:client`

**Checklists** - [x] Added impacted package name to the issue description **Packages impacted by this PR:** @azure-rest/agrifood-farming **Describe the problem that is addressed by this PR:** Re-generate Agrifood Farming using the latest version of RLC generator. This package was one of the first-ever RLCs generated and codegen has changes quite a bit since then. Regenerating against the same swagger. Major changes since last generation: - Switched away from @azure-rest/core-client-lro and @azure-rest/core-client-paging in favor of @azure/core-lro and @azure/core-client - Split Output and Input models **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)_ NO **Are there test cases added in this PR?**_(If not, why?)_ NO, it already has tests **Provide a list of related PRs**_(if any)_ Azure#19897 **Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ `rushx generate:client`

Re-generate Agrifood and deprecate old packages

a194abe

joheredi requested review from benbp, bterlson, deyaaeldeen, jeremymeng, mikeharder, praveenkuttappan, qiaozha, ramya-rao-a, witemple-msft and xirzec as code owners January 18, 2022 20:05

ghost added Azure.Core Purview AgriFood labels Jan 18, 2022

xirzec reviewed Jan 18, 2022

View reviewed changes

Remove references to deprecated packages

7de1537

joheredi requested a review from weshaggard as a code owner January 19, 2022 00:23

Fix lint

2dbbe3b

witemple-msft reviewed Jan 19, 2022

View reviewed changes

joheredi mentioned this pull request Jan 25, 2022

[Agrifood Farming] Regenerate with latest codegen #20059

Merged

1 task

joheredi closed this Jan 25, 2022

Conversation

joheredi commented Jan 18, 2022

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azure-sdk commented Jan 19, 2022

Uh oh!

witemple-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joheredi Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joheredi commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joheredi Jan 21, 2022 •

edited

Loading