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.

Sign up for GitHub

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

Jump to bottom

[KeyVault] - Use shared rollup config #19091

Merged
merged 10 commits into from
Dec 10, 2021
Merged

[KeyVault] - Use shared rollup config #19091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9d8e697
keyvault-admin
maorleger Dec 9, 2021
0ee37b8
keyvault-keys
maorleger Dec 9, 2021
388793a
fixup! keyvault-admin
maorleger Dec 9, 2021
5837440
support <package>-common cases in rollup
maorleger Dec 10, 2021
619fb08
keyvault-certificates
maorleger Dec 10, 2021
6602111
keyvault-secrets
maorleger Dec 10, 2021
9be2e81
quiet opentelemetry's spreadarray warnings
maorleger Dec 10, 2021
fc8cd87
update usage of makeBrowserTestConfig
maorleger Dec 10, 2021
80f36df
make pkg optional
maorleger Dec 10, 2021
b856ea3
fixup! make pkg optional
maorleger Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions common/tools/dev-tool/src/config/rollup.base.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import cjs from "@rollup/plugin-commonjs";
import sourcemaps from "rollup-plugin-sourcemaps";
import multiEntry from "@rollup/plugin-multi-entry";
import json from "@rollup/plugin-json";
import * as path from "path";

import nodeBuiltins from "builtin-modules";

Expand Down Expand Up @@ -83,9 +84,16 @@ function ignoreChaiCircularDependencyWarnings(warning: RollupWarning): boolean {
);
}

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

Comment on lines +87 to +92
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const warningInhibitors: Array<(warning: RollupWarning) => boolean> = [
ignoreChaiCircularDependencyWarnings,
ignoreNiseSinonEvalWarnings
ignoreNiseSinonEvalWarnings,
ignoreOpenTelemetryThisIsUndefinedWarnings
];

/**
Expand All @@ -103,11 +111,16 @@ function makeOnWarnForTesting(): (warning: RollupWarning, warn: WarningHandler)

// #endregion

export function makeBrowserTestConfig(): RollupOptions {
export function makeBrowserTestConfig(pkg: PackageJson): RollupOptions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! good call, thanks 😄

Copy link
Member Author

@maorleger maorleger Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

// ./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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const basePath = path.dirname(path.parse(module).dir);

const config: RollupOptions = {
input: {
include: ["dist-esm/test/**/*.spec.js"],
exclude: ["dist-esm/test/**/node/**"]
include: [path.join(basePath, "test", "**", "*.spec.js")],
exclude: [path.join(basePath, "test", "**", "node", "**")]
},
output: {
file: `dist-test/index.browser.js`,
Expand Down Expand Up @@ -173,7 +186,7 @@ export function makeConfig(pkg: PackageJson, options?: Partial<ConfigurationOpti
const config: RollupOptions[] = [baseConfig as RollupOptions];

if (!options.disableBrowserBundle) {
config.push(makeBrowserTestConfig());
config.push(makeBrowserTestConfig(pkg));
}

return config;
Expand Down
13 changes: 2 additions & 11 deletions sdk/keyvault/keyvault-admin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"build:node": "tsc -p . && cross-env ONLY_NODE=true rollup -c 2>&1",
"build:browser": "tsc -p . && cross-env ONLY_BROWSER=true rollup -c 2>&1",
"build:nodebrowser": "rollup -c 2>&1",
"build:test": "tsc -p . && rollup -c rollup.test.config.js 2>&1",
"build:test": "tsc -p . && rollup -c 2>&1",
"build": "npm run clean && tsc -p . && npm run build:nodebrowser && api-extractor run --local",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"samples-dev/**/*.ts\" \"*.{js,json}\"",
"clean": "rimraf dist dist-* types *.tgz *.log statistics.html coverage && rimraf src/**/*.js && rimraf test/**/*.js",
Expand All @@ -66,7 +66,7 @@
"test:node": "npm run clean && npm run build:test && npm run unit-test:node",
"test": "npm run clean && npm run build:test && npm run unit-test",
"unit-test:browser": "echo skipped",
"unit-test:node": "mocha --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 180000 --full-trace \"dist-test/index.node.js\"",
"unit-test:node": "mocha -r esm --require ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 180000 --full-trace \"test/{,!(browser)/**/}*.spec.ts\"",
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"docs": "typedoc --excludePrivate --excludeNotExported --excludeExternals --stripInternal --mode file --out ./dist/docs ./src"
},
Expand Down Expand Up @@ -125,11 +125,6 @@
"@azure/test-utils": "^1.0.0",
"@azure-tools/test-recorder": "^1.0.0",
"@microsoft/api-extractor": "^7.18.11",
"@rollup/plugin-commonjs": "11.0.2",
"@rollup/plugin-json": "^4.0.0",
"@rollup/plugin-multi-entry": "^3.0.0",
"@rollup/plugin-node-resolve": "^8.0.0",
"@rollup/plugin-replace": "^2.2.0",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.0",
"@types/mocha": "^7.0.2",
Expand All @@ -148,10 +143,6 @@
"prettier": "^1.16.4",
"rimraf": "^3.0.0",
"rollup": "^1.16.3",
"rollup-plugin-shim": "^1.0.0",
"rollup-plugin-sourcemaps": "^0.4.2",
"rollup-plugin-terser": "^5.1.1",
"rollup-plugin-visualizer": "^4.0.4",
"sinon": "^9.0.2",
"source-map-support": "^0.5.9",
"typedoc": "0.15.2",
Expand Down
152 changes: 0 additions & 152 deletions sdk/keyvault/keyvault-admin/rollup.base.config.js
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions sdk/keyvault/keyvault-admin/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import * as base from "./rollup.base.config";
import { makeConfig } from "@azure/dev-tool/shared-config/rollup";

export default [base.nodeConfig()];
export default makeConfig(require("./package.json"));
6 changes: 0 additions & 6 deletions sdk/keyvault/keyvault-admin/rollup.test.config.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion sdk/keyvault/keyvault-admin/test/internal/userAgent.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe("Key Vault Admin's user agent (only in Node, because of fs)", function(
let version: string;
try {
const fileContents = JSON.parse(
fs.readFileSync(path.join(__dirname, "../package.json"), { encoding: "utf-8" })
fs.readFileSync(path.join(__dirname, "../../package.json"), { encoding: "utf-8" })
);
version = fileContents.version;
} catch {
Expand Down
4 changes: 2 additions & 2 deletions sdk/keyvault/keyvault-certificates/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"build:node": "tsc -p . && cross-env ONLY_NODE=true rollup -c 2>&1",
"build:browser": "tsc -p . && cross-env ONLY_BROWSER=true rollup -c 2>&1",
"build:nodebrowser": "rollup -c 2>&1",
"build:test": "tsc -p . && rollup -c rollup.test.config.js 2>&1",
"build:test": "tsc -p . && rollup -c 2>&1",
"build": "npm run clean && tsc -p . && npm run build:nodebrowser && api-extractor run --local",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"samples-dev/**/*.ts\" \"*.{js,json}\"",
"clean": "rimraf dist-esm dist-test types *.tgz *.log samples/typescript/dist",
Expand All @@ -63,7 +63,7 @@
"test:node": "npm run clean && npm run build:test && npm run unit-test:node",
"test": "npm run clean && npm run build:test && npm run unit-test",
"unit-test:browser": "karma start --single-run",
"unit-test:node": "mocha --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 300000 --full-trace \"dist-test/index.node.js\"",
"unit-test:node": "mocha -r esm --require ts-node/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 300000 --full-trace \"test/{,!(browser)/**/}*.spec.ts\"",
"unit-test:node:no-timeout": "mocha --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --no-timeouts --full-trace \"dist-test/index.node.js\"",
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"docs": "typedoc --excludePrivate --excludeNotExported --excludeExternals --stripInternal --mode file --out ./dist/docs ./src"
Expand Down
Loading