Initial creation of Azure DigitalTwins SDK #9787

zolvarga · 2020-06-29T16:03:15Z

No description provided.

xirzec

I'm not an expert in what this package is trying to accomplish, so apologies if I misunderstood parts of it. Most of my comments are regarding how this package doesn't quite conform to what we require of a data-plane track 2 client.

xirzec · 2020-06-29T19:53:14Z

sdk/digitaltwins/digitaltwins/README.md

+
+## Getting started
+
+The complete Microsoft Azure SDK can be downloaded from the [Microsoft Azure downloads][microsoft_sdk_download] page, and it ships with support for building deployment packages, integrating with tooling, rich command line tooling, and more.


It feels a little weird to link off to downloading the SDK when we're in the GitHub for the SDK

This section points to the central download page but the next section provides the npm install instructions. I think both has value.

In reply to: 447215310 [](ancestors = 447215310)

sdk/digitaltwins/digitaltwins/README.md

xirzec · 2020-06-29T19:54:31Z

sdk/digitaltwins/digitaltwins/README.md

+<!-- LINKS -->
+
+[microsoft_sdk_download]: https://azure.microsoft.com/en-us/downloads/?sdk=net
+[azure_sdk_target_frameworks]: https://github.com/azure/azure-sdk-for-net#target-frameworks


link unused

Removed.

In reply to: 447216052 [](ancestors = 447216052)

xirzec · 2020-06-29T19:54:54Z

sdk/digitaltwins/digitaltwins/README.md

+[azure_sub]: https://azure.microsoft.com/free/
+[source]: https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/digitaltwins
+[package]: https://www.npmjs.com/package/@azure/digitaltwins
+[adt_npm]: https://www.npmjs.com/package/@azure/digitaltwins


do we need two links for the same URL?

Removed

In reply to: 447216356 [](ancestors = 447216356)

xirzec · 2020-06-29T19:55:12Z

sdk/digitaltwins/digitaltwins/README.md

+[npm]: https://www.npmjs.com/
+[azure_portal]: https://portal.azure.com/
+[azure_rest_api]: https://docs.microsoft.com/en-us/rest/api/azure/
+[azure_core_library]: https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/core/Azure.Core


Updated usage

In reply to: 447216533 [](ancestors = 447216533)

xirzec · 2020-06-29T21:22:57Z

sdk/digitaltwins/digitaltwins/src/digitalTwinsClient.ts

+    digitalTwinId: string,
+    options: DigitalTwinsListRelationshipsOptionalParams
+  ): AsyncIterableIterator<any> {
+    const f = {};


what is this argument?

Removed

In reply to: 447262020 [](ancestors = 447262020)

xirzec · 2020-06-29T21:24:39Z

sdk/digitaltwins/digitaltwins/src/digitalTwinsClient.ts

+    maxItemCount: number = -1
+  ): PagedAsyncIterableIterator<EventRoute, EventRoutesListNextResponse> {
+    var options: EventRoutesListOptionalParams & PageSettings = {};
+    if (maxItemCount != -1) {


lint: should use strict equality checks

Suggested change

if (maxItemCount != -1) {

if (maxItemCount !== -1) {

If the linter isn't running, you may have to tweak .eslintrc.json

Fixed

In reply to: 447263018 [](ancestors = 447263018)

Fixed

In reply to: 447262868 [](ancestors = 447262868)

xirzec · 2020-06-29T21:25:45Z

sdk/digitaltwins/digitaltwins/src/digitalTwinsClient.ts

+  }
+}
+
+export {


typically we have a /src/index.ts that handles all top level package exports instead of the client doing this

It is in the ToDo list

In reply to: 447263479 [](ancestors = 447263479)

xirzec · 2020-06-29T21:27:17Z

sdk/digitaltwins/digitaltwins/package.json

+    "pack": "npm pack 2>&1",
+    "build": "tsc -p . && rollup -c 2>&1 && api-extractor run --local",
+    "extract-api": "tsc -p . && api-extractor run --local",
+    "generate-pl": "autorest --typescript --typescript.azure-arm=true --use=@microsoft.azure/autorest.typescript@5.1.0 --add-credentials --model-enum-as-union --license-header=MICROSOFT_MIT_NO_VERSION --output-folder=./src/generated --input-file=swagger/digitaltwins.json"


do we need --typescript.azure-arm=true ?

Yes we do. The paginated API generation is missing if we don't specify "arm" here. (Chris Radek figured it out)

In reply to: 447264176 [](ancestors = 447264176)

xirzec · 2020-06-29T21:28:17Z

sdk/digitaltwins/digitaltwins/swagger/digitaltwins.json

@@ -0,0 +1,1497 @@
+{


why is the swagger being checked into this repo? I think it should live here instead: https://github.com/Azure/azure-rest-api-specs/

I agree and eventually it will be only in azure-rest-api-spec. But until we don't have a way to auto-generate from there, I followed the example of the digitaltwins .NET SDK and checked into this repo. (They have the swagger checked in to azure SDK folder) If you strongly against that I will remove it from here. It is not a big deal.

In reply to: 447264662 [](ancestors = 447264662)

I'm not sure about auto-generate, but we simply reference the external swagger by URL in other client libraries.

Yes, but if there is no auto-generate in the case of new version of swagger it is better to keep the original swagger what is the code generated from, is'n it?

In reply to: 453892297 [](ancestors = 453892297)

That would make sense to me, except what about generating the other language SDKs? Having the spec live in some language neutral place has the advantage of allowing all languages to reference the same spec.

For cognitive search we used that to drive changes across languages (e.g. the service team updated the swagger in one place then we regenerated all clients.)

Removed

In reply to: 453986003 [](ancestors = 453986003)

Removed

In reply to: 453977418 [](ancestors = 453977418,453892297)

Removed

In reply to: 453758091 [](ancestors = 453758091,447264662)

…to digitaltwins

zolvarga · 2020-07-14T22:42:05Z

Thanks for the comments, all of them valuable. We will discuss what is the purpose of this service in a meeting.

In reply to: 439452976 [](ancestors = 439452976)

bterlson

Overall looking great, couple suggestions.

bterlson · 2020-07-15T20:25:27Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+}
+
+// @public
+export interface DigitalTwinModelsAddOptionalParams extends coreHttp.RequestOptionsBase {


This should extend OperationOptions

bterlson · 2020-07-15T20:25:56Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+
+// @public
+export type DigitalTwinModelsAddResponse = Array<ModelData> & {
+    _response: coreHttp.HttpResponse & {


_response is good.

bterlson · 2020-07-15T20:27:21Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+
+// @public
+export interface DigitalTwinsAddHeaders {
+    eTag: string;


js prefers etag.

This would be a swagger change but it would effect all languages.

In reply to: 455321769 [](ancestors = 455321769)

I don't know if swagger has an "etag" type, but in the least it could be a fix to autorest code generator to use etag type if the parameter name is etag.

As a data point, currently in the data plane swagger, the etag header is spelled "ETag" (capital E and capital T). This looks consistent with the spelling in the RFC (RFC 7232) and places like Wikipedia.

bterlson · 2020-07-15T20:27:42Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+};
+
+// @public
+export interface DigitalTwinModelsListOptionalParams extends coreHttp.RequestOptionsBase {


Generally replace ROB with OperationOptions.

bterlson · 2020-07-15T20:28:48Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+};
+
+// @public
+export class DigitalTwinsClient {


One thing we should discuss is whether it makes sense to have separate clients for models, twins, components, and relationships.

bterlson · 2020-07-15T20:31:37Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+    publishComponentTelemetry(digitalTwinId: string, componentPath: string, payload: string, messageId?: string): Promise<RestResponse>;
+    publishTelemetry(digitalTwinId: string, payload: any, messageId?: string): Promise<RestResponse>;
+    queryTwins(query?: string): PagedAsyncIterableIterator<any, QueryQueryTwinsResponse>;
+    updateComponent(digitalTwinId: string, componentPath: string, componentPatch: any, ifMatch?: string): Promise<DigitalTwinsUpdateComponentResponse>;


componentPatch can likely be typed more precisely than any. Even object might be preferable if we can't be more specific.

bterlson · 2020-07-15T20:33:20Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+
+// @public
+export interface DigitalTwinsSendComponentTelemetryOptionalParams extends coreHttp.RequestOptionsBase {
+    dtTimestamp?: string;


dtTimestamp doesn't seem super descriptive to me.

We discussed: this comes from the swagger and we have a work item to improve this name for ADT GA.

In reply to: 455324864 [](ancestors = 455324864)

FWIW, in the .NET client, we called this TimeStamp for now, and will update it along with the swagger change at GA.

Added to the swagger changes list

In reply to: 455388544 [](ancestors = 455388544,455324864)

bterlson · 2020-07-15T20:33:59Z

sdk/digitaltwins/digitaltwins/review/digitaltwins.api.md

+export type IfNoneMatch = '*';
+
+// @public
+export type IfNoneMatch1 = '*';


Duplicate declaration here.

Added to the swagger changes list

In reply to: 455325165 [](ancestors = 455325165)

sdk/digitaltwins/digitaltwins/README.md

sdk/digitaltwins/digitaltwins/samples/Readme.md

xirzec · 2020-07-15T22:45:41Z

sdk/digitaltwins/digitaltwins/samples/javascript/dt_digitaltwins_lifecycle.js

+  const createdTwin = await serviceClient.upsertDigitalTwin(
+    digitalTwinId,
+    newTwin,
+    (enableUpdate = true)


Per review: we should avoid this pattern in our code. The usual recommendation is to use option bags so you can write {enableUpdate: true}.

When that can't be used, it's accepted to use comments to name boolean params e.g. doFoo(/*forceUpdate*/ true)

Fixed

In reply to: 455407628 [](ancestors = 455407628)

drwill-ms · 2020-07-20T22:23:23Z

sdk/digitaltwins/digitaltwins/README.md

+
+### Install the package
+
+Install the Azure Digital Twins client library for Javascript with [NPM][npm_package]:


capital S in JavaScript?

ramya-rao-a · 2020-07-23T01:36:19Z

@zolvarga Do you mind updating the PR description with a link to Azure/azure-sdk#1499 for other reviewers stumbling on this?

zolvarga · 2020-08-11T16:16:28Z

Closing as moved to final review

Initial creation of Azure DigitalTwins SDK

1402bb3

zolvarga requested review from bterlson, ramya-rao-a and xirzec as code owners June 29, 2020 16:03

zolvarga mentioned this pull request Jun 29, 2020

Board Review: Azure DigitalTwins for Typescript Azure/azure-sdk#1499

Closed

xirzec reviewed Jun 29, 2020

View reviewed changes

Zoltan Varga added 2 commits July 13, 2020 09:55

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

ff43560

…to digitaltwins

chore<@azure/digitaltwins>: address comments

8842134

drwill-ms self-requested a review July 14, 2020 22:25

chore<digitaltinws>: address comments

258b264

bterlson approved these changes Jul 15, 2020

View reviewed changes

drwill-ms reviewed Jul 15, 2020

View reviewed changes

sdk/digitaltwins/digitaltwins/README.md Outdated Show resolved Hide resolved

drwill-ms reviewed Jul 15, 2020

View reviewed changes

sdk/digitaltwins/digitaltwins/README.md Outdated Show resolved Hide resolved

drwill-ms reviewed Jul 15, 2020

View reviewed changes

sdk/digitaltwins/digitaltwins/samples/Readme.md Outdated Show resolved Hide resolved

drwill-ms reviewed Jul 15, 2020

View reviewed changes

sdk/digitaltwins/digitaltwins/samples/Readme.md Outdated Show resolved Hide resolved

xirzec reviewed Jul 15, 2020

View reviewed changes

chore(digitaltwins): address comments

fc0d588

drwill-ms reviewed Jul 20, 2020

View reviewed changes

Zoltan Varga added 2 commits July 20, 2020 15:55

feat(digitaltwins): add external swagger location

56cfb1c

chore(digitaltwins): fix swagger output folder

af3bfec

Zoltan Varga added 2 commits July 23, 2020 15:23

chore(digitaltwins); replace eTag, move gen code

1dcba8e

chore(digitaltwins): add unit tests

4f988a2

zolvarga requested review from KarishmaGhiya and mikeharder as code owners July 30, 2020 23:16

Zoltan Varga added 4 commits July 31, 2020 11:15

chore(digitaltwins): check in for investigation

7e5e80f

feat(digitaltwins): update autorest

1a994b5

fix(digitaltwins): auth fix and samples

286592b

feat(digitaltwins): add more sample

8ebaee2

chore(digitaltwins): save it for investigation

0671252

zolvarga requested a review from danieljurek as a code owner August 7, 2020 17:05

feat(digitaltwins): enable test in browser

6f82cbd

zolvarga closed this Aug 11, 2020


		## Getting started

		The complete Microsoft Azure SDK can be downloaded from the [Microsoft Azure downloads][microsoft_sdk_download] page, and it ships with support for building deployment packages, integrating with tooling, rich command line tooling, and more.


		### Install the package

		Install the Azure Digital Twins client library for Javascript with [NPM][npm_package]:

Initial creation of Azure DigitalTwins SDK #9787

Initial creation of Azure DigitalTwins SDK #9787

Conversation

zolvarga commented Jun 29, 2020

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolvarga Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolvarga Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolvarga commented Jul 14, 2020

bterlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jul 23, 2020 • edited Loading

zolvarga commented Aug 11, 2020

zolvarga Jul 14, 2020 •

edited

Loading

zolvarga Jul 13, 2020 •

edited

Loading

ramya-rao-a commented Jul 23, 2020 •

edited

Loading