[KeyVault] - Use shared rollup config #19091

maorleger · 2021-12-10T00:31:41Z

What

Migrate keyvault to use the shared rollup config
Teach the shared config to handle <package>-common shared-source pattern
Suppress this has been rewritten to undefined errors for OpenTelemetry
Replace usages of assert with chai

Why

We're migrating packages over to the shared config, and this handles Key Vault packages.

Key Vault is the first migrated package that includes the shared-source keyvault-common pattern, and as such
it does not conform to the file structure rollup config expects. So we need to teach it to be a little smarter.

Finally, OTel's codebase emits the spreadArray helper var __spreadArray = (this && this.__spreadArray) || ... which
causes Rollup to be unhappy as the top-level this is undefined. In this case (couldn't resist) it's harmless and
can be suppressed.

Finally, we are removing assert entirely so it doesn't make sense to add it to the namedExports collection - instead
I am just replacing it with chai wherever I encounter it.

Resolves #13410

deyaaeldeen · 2021-12-10T00:47:35Z

common/tools/dev-tool/src/config/rollup.base.config.ts

+function ignoreOpenTelemetryThisIsUndefinedWarnings(warning: RollupWarning): boolean {
+  return (
+    warning.code === "THIS_IS_UNDEFINED" && warning.id?.includes("@opentelemetry/api") === true
+  );
+}
+


is this warning the same one that was handled by openTelemetryCommonJs?

This is a different one:

dist-esm/test/**/*.spec.js, dist-esm/test/**/node/** → dist-test/index.browser.js... (!) `this` has been rewritten to `undefined` https://rollupjs.org/guide/en/#error-this-is-undefined ../../../common/temp/node_modules/.pnpm/@opentelemetry+api@1.0.3/node_modules/@opentelemetry/api/build/esm/api/context.js 14: * limitations under the License. 15: */ 16: var __spreadArray = (this && this.__spreadArray) || function (to, from) { ^ 17: for (var i = 0, il = from.length, j = to.length; i < il; i++, j++) 18: to[j] = from[i]; ...and 1 other occurrence ../../../common/temp/node_modules/.pnpm/@opentelemetry+api@1.0.3/node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js 14: * limitations under the License. 15: */ 16: var __spreadArray = (this && this.__spreadArray) || function (to, from) { ^ 17: for (var i = 0, il = from.length, j = to.length; i < il; i++, j++) 18: to[j] = from[i]; ...and 1 other occurrence

In this case this is undefined is fine, since the tslib helper that's emitted (At least that's what I think is happening here - correct me if I'm wrong) is valid JS and won't cause runtime issues.

I wonder why it's being emitted from OTel - but given we are going to be removing OTel as a dependency of our SDK I don't know if it's worth investigating further - what do you think?

I think your synopsis is correct and that we can suppress this warning. It's a warning because sometimes it's a problem and sometimes it's not. In this case it's not because top-level this is just part of the polyfill of array spreading in tslib.

It must choose to emit this helper because the OTel compile target is not ES6 or higher. This is a little troubling because it likely means that we are not picking up the module field of the OTel API package. Their CommonJS files are ES5, but ESMs are ES6.

deyaaeldeen · 2021-12-10T00:50:28Z

sdk/keyvault/keyvault-certificates/test/internal/userAgent.spec.ts

@@ -23,7 +23,7 @@ describe("Certificates client's user agent (only in Node, because of fs)", () =>
    try {
      // The unit-test script has this test file at: test/internal/userAgent.spec.ts
      const fileContents = JSON.parse(
-        fs.readFileSync(path.join(__dirname, "../package.json"), { encoding: "utf-8" })
+        fs.readFileSync(path.join(__dirname, "../../package.json"), { encoding: "utf-8" })


huh, I wonder why it was passing before.

likely the bundled nodejs test goes through the catch block below?

deyaaeldeen · 2021-12-10T00:52:38Z

common/tools/dev-tool/src/config/rollup.base.config.ts

@@ -103,11 +111,15 @@ function makeOnWarnForTesting(): (warning: RollupWarning, warn: WarningHandler)

 // #endregion

-export function makeBrowserTestConfig(): RollupOptions {
+export function makeBrowserTestConfig(pkg: PackageJson): RollupOptions {


This function is exported, could you update all calls to it? there is one call in schema registry.

Oops! good call, thanks 😄

I updated it at the call because that's the only one I found and seemed to be a one-off case - but I was debating whether to make pkg an optional param and default to dist-esm/src/index.js if it's not provided like in https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/dev-tool/src/config/rollup.base.config.ts#L162

edit: looking at it again, I guess if base config uses a default value if pkg["module"] does not exist maybe I should too...

What do you all think?

edit: looking at it again, I guess if base config uses a default value if pkg["module"] does not exist maybe I should too...

Sounds good to me! Thanks!

witemple-msft

Sounds sensible to me.

witemple-msft · 2021-12-10T19:03:58Z

common/tools/dev-tool/src/config/rollup.base.config.ts

+export function makeBrowserTestConfig(pkg: PackageJson): RollupOptions {
+  // ./dist-esm/src/index.js -> ./dist-esm
+  // ./dist-esm/keyvault-keys/src/index.js -> ./dist-esm/keyvault-keys
+  const module = pkg["module"] ?? "dist-esm/src/index.js";


There are only two packages that don't have module:

core-asynciterator-polyfill

mock-hub

The rest that don't have it are perf-tests and samples.

This default was really from a different time and I'd be fine with throwing an error in this case now assuming it doesn't break anything unexpected.

The only way I can see this logic breaking is if the index module isn't in the same place relative to the test folder in any package. That is something that might be worth just spot-checking to make sure we don't break a random package with this change.

[Hub Generated] Review request for Microsoft.Network to add version stable/2022-05-01 (Azure#19091) * Adds base for updating Microsoft.Network from version stable/2020-11-01 to version 2022-05-01 * Updates readme * Updates API version in new specs and examples * Add frontdoor waf Patch version 2022-05-01 * Adds suppression to readme * Adds suppression to readme * Fix format errors * Fix validation issues * Fix ModelValidation error * fix valiation errors * Adds suppression to readme * Fix valiation error * Fix lintdiff error * Fix lintdiff error * Fix lintdiff error * Fix code format * Fix DefaultErrorResponseSchema lintDiff * Fix PrettierCheck error * Change ErrorResponse schema * Update network.json * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * suppress LintDiff

maorleger added 7 commits December 9, 2021 13:43

keyvault-admin

9d8e697

keyvault-keys

0ee37b8

fixup! keyvault-admin

388793a

support <package>-common cases in rollup

5837440

keyvault-certificates

619fb08

keyvault-secrets

6602111

quiet opentelemetry's spreadarray warnings

9be2e81

maorleger requested review from deyaaeldeen, mikeharder, praveenkuttappan, sadasant and witemple-msft as code owners December 10, 2021 00:31

ghost added KeyVault dev-tool Issues related to the Azure SDK for JS dev-tool labels Dec 10, 2021

maorleger requested a review from JonathanCrd December 10, 2021 00:38

deyaaeldeen reviewed Dec 10, 2021

View reviewed changes

update usage of makeBrowserTestConfig

fc8cd87

maorleger requested a review from nguerrera as a code owner December 10, 2021 17:41

maorleger added 2 commits December 10, 2021 09:56

make pkg optional

80f36df

fixup! make pkg optional

b856ea3

witemple-msft approved these changes Dec 10, 2021

View reviewed changes

maorleger merged commit 6477870 into Azure:main Dec 10, 2021

maorleger deleted the shared-rollup-config branch December 10, 2021 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KeyVault] - Use shared rollup config #19091

[KeyVault] - Use shared rollup config #19091

maorleger commented Dec 10, 2021

deyaaeldeen Dec 10, 2021

maorleger Dec 10, 2021

witemple-msft Dec 10, 2021

deyaaeldeen Dec 10, 2021

jeremymeng Dec 10, 2021

deyaaeldeen Dec 10, 2021

maorleger Dec 10, 2021

maorleger Dec 10, 2021 •

edited

Loading

deyaaeldeen Dec 10, 2021

witemple-msft left a comment

witemple-msft Dec 10, 2021

witemple-msft Dec 10, 2021

[KeyVault] - Use shared rollup config #19091

[KeyVault] - Use shared rollup config #19091

Conversation

maorleger commented Dec 10, 2021

What

Why

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

maorleger Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witemple-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger Dec 10, 2021 •

edited

Loading