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

[dev-tool] Shared rollup config factory #10923

Merged
merged 20 commits into from
Sep 28, 2020
Merged

[dev-tool] Shared rollup config factory #10923

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
81d1203
WIP
witemple-msft Aug 10, 2020
685f8d1
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Aug 28, 2020
015266f
[dev-tool] Abstract rollup config factory
witemple-msft Aug 28, 2020
25af0f1
fix typo
witemple-msft Aug 29, 2020
33889bf
Fixed typeck error
witemple-msft Aug 31, 2020
9dffc29
dependency cleanup
witemple-msft Aug 31, 2020
ac2edcb
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Aug 31, 2020
ff11f10
Removed errant test config from template
witemple-msft Sep 1, 2020
3b89784
Fixed template karma configuration
witemple-msft Sep 1, 2020
d19da79
Removed ghost files, small tweak to base config
witemple-msft Sep 12, 2020
6b5b4bb
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Sep 14, 2020
641441b
Removed explicit config env from template package.json
witemple-msft Sep 14, 2020
8b79559
Removed IS_PRODUCTION gates
witemple-msft Sep 15, 2020
c6b182d
Removed dependency on terser
witemple-msft Sep 15, 2020
0c96ff2
Removed dependency on fs-extra
witemple-msft Sep 15, 2020
958e67a
Added flag to disable browser test bundle
witemple-msft Sep 15, 2020
65839dc
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Sep 16, 2020
c737534
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Sep 18, 2020
9820cef
Removed karma-typescript
witemple-msft Sep 24, 2020
3e9bf49
Merge remote-tracking branch 'upstream/master' into rollup-abstraction
witemple-msft Sep 24, 2020
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
2,819 changes: 1,806 additions & 1,013 deletions common/config/rush/pnpm-lock.yaml
View file
Open in desktop

Large diffs are not rendered by default.

71 changes: 71 additions & 0 deletions common/tools/dev-tool/'
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
{
willmtemple marked this conversation as resolved.
Show resolved Hide resolved
"name": "@azure/dev-tool",
"version": "1.0.0",
"description": "A helpful command for azure-sdk-for-js developers",
"bin": {
"dev-tool": "launch.js"
},
"files": [
"src"
],
"scripts": {
"audit": "node ../../../common/scripts/rush-audit.js && rimraf node_modules package-lock.json && npm i --package-lock-only 2>&1 && npm audit",
"build": "tsc",
"build:test": "echo skipped",
"clean": "rimraf dist dist-* *.tgz *.log",
"extract-api": "echo skipped",
"format": "prettier --write src/**/*.ts test/**/*.ts *.{js,json}",
"integration-test:browser": "echo skipped",
"integration-test:node": "echo skipped",
"lint": "eslint src test --ext .ts -f html -o template-lintReport.html || exit 0",
"pack": "npm pack 2>&1",
"prebuild": "npm run clean",
"unit-test": "mocha --require ts-node/register test/**/*.spec.ts"
},
"repository": "github:Azure/azure-sdk-for-js",
"author": "Microsoft Corporation",
"license": "MIT",
"bugs": {
"url": "https://github.com/azure/azure-sdk-for-js/issues"
},
"homepage": "https://github.com/azure/azure-sdk-for-js/tree/master/common/tools/dev-tool",
"sideEffects": false,
"private": true,
"prettier": "@azure/eslint-plugin-azure-sdk/prettier.json",
"dependencies": {
"chalk": "~3.0.0",
"fs-extra": "^8.1.0",
"minimist": "~1.2.5",
"prettier": "^1.16.4",
"ts-node": "^8.3.0",
"typescript": "~3.9.3"
},
"devDependencies": {
"@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",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.0",
"@types/chalk": "~2.2.0",
"@types/fs-extra": "^8.0.0",
"@types/minimist": "~1.2.0",
"@types/mocha": "^7.0.2",
"@types/node": "^8.0.0",
"@types/prettier": "~2.0.1",
"@typescript-eslint/eslint-plugin": "^2.0.0",
"@typescript-eslint/parser": "^2.0.0",
"builtin-modules": "~3.1.0",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"eslint": "^6.1.0",
"mocha": "^7.1.1",
"rimraf": "^3.0.0",
"rollup": "^1.16.3",
"rollup-plugin-node-builtins": "~2.1.2",
"rollup-plugin-node-globals": "~1.4.0",
"rollup-plugin-sourcemaps": "^0.4.2",
"rollup-plugin-terser": "^5.1.1",
"rollup-plugin-visualizer": "^4.0.4"
}
}
13 changes: 12 additions & 1 deletion common/tools/dev-tool/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@
},
"devDependencies": {
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@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",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.0",
"@types/chalk": "~2.2.0",
Expand All @@ -52,10 +56,17 @@
"@types/prettier": "~2.0.1",
"@typescript-eslint/eslint-plugin": "^2.0.0",
"@typescript-eslint/parser": "^2.0.0",
"builtin-modules": "~3.1.0",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"eslint": "^6.1.0",
"mocha": "^7.1.1",
"rimraf": "^3.0.0"
"rimraf": "^3.0.0",
"rollup": "^1.16.3",
"rollup-plugin-node-builtins": "~2.1.2",
"rollup-plugin-node-globals": "~1.4.0",
"rollup-plugin-sourcemaps": "^0.4.2",
"rollup-plugin-terser": "^5.1.1",
"rollup-plugin-visualizer": "^4.0.4"
}
}
14 changes: 14 additions & 0 deletions common/tools/dev-tool/shared-config/rollup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// This shim file simply loads ts-node and then requires the
// TypeScript file corresponding to the rollup base config

const { join } = require("path");

require("ts-node").register({
transpileOnly: true,
project: join(__dirname, "../tsconfig.json")
});

module.exports = require(join(__dirname, "../src/config/rollup.base.config.ts"));
32 changes: 32 additions & 0 deletions common/tools/dev-tool/src/ambient.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright (c) Microsoft Corporation
// Licensed under the MIT license.

/**
* Some delcarations for plugins that have missing/old declarations in
* NPM.
*/

// #region rollup

type Plugin = import("rollup").Plugin;

declare module "rollup-plugin-sourcemaps" {
export default function(): Plugin;
}

declare module "rollup-plugin-node-globals" {
export default function(): Plugin;
}

declare module "rollup-plugin-node-builtins" {
export default function(): Plugin;
}

declare module "@rollup/plugin-multi-entry" {
interface MultiEntryOptions {
exports: boolean;
}
export default function(opts: MultiEntryOptions): Plugin;
}

// #endregion
118 changes: 118 additions & 0 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
@@ -0,0 +1,118 @@
import type { RollupOptions, RollupWarning, WarningHandler } from "rollup";

import nodeResolve from "@rollup/plugin-node-resolve";
import cjs from "@rollup/plugin-commonjs";
import { terser } from "rollup-plugin-terser";
import sourcemaps from "rollup-plugin-sourcemaps";
import multiEntry from "@rollup/plugin-multi-entry";
import json from "@rollup/plugin-json";
import nodeBuiltinsPlugin from "rollup-plugin-node-builtins";
import nodeGlobals from "rollup-plugin-node-globals";

import nodeBuiltins from "builtin-modules";

const IS_PRODUCTION = process.env.NODE_ENV === "production";

interface PackageJson {
name: string;
dependencies: Record<string, string>;
devDependencies: Record<string, string>;
}

// #region Warning Handler

function ignoreNiseSinonEvalWarnings(warning: RollupWarning): boolean {
return warning.code === "EVAL" && (
warning.id?.includes("node_modules/nise") ||
warning.id?.includes("node_modules/sinon")) === true;
}

function ignoreChaiCircularDependencyWarnings(warning: RollupWarning): boolean {
return warning.code === "CIRCULAR_DEPENDENCY" &&
warning.importer?.includes("node_modules/chai") === true;
}

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

/**
* Construct a warning handler for the shared rollup configuration
* that ignores certain warnings that are not relevant to testing.
*/
function makeOnWarnForTesting(): (warning: RollupWarning, warn: WarningHandler) => void {
return (warning, warn) => {
// If every inhibitor returns false (i.e. no inhibitors), then show the warning
if (warningInhibitors.every((inhib) => !inhib(warning))) {
warn(warning);
}
}
}

// #endregion

function makeBrowserTestConfig() {
const config: RollupOptions = {
input: ["dist-esm/test/{,!(node)/**/}*.spec.js"],
output: {
file: `dist-test/index.browser.js`,
format: "umd",
sourcemap: true,
globals: { "fs-extra": "undefined" }
},
preserveSymlinks: false,
// fs-extra must be marked as external in order to avoid an initialization error
external: ["fs-extra"],
plugins: [
multiEntry({ exports: false }),
nodeResolve({
mainFields: ["module", "browser"],
preferBuiltins: true
}),
cjs({
namedExports: {
"chai": ["assert", "use"],
"@opentelemetry/api": ["CanonicalCode", "SpanKind", "TraceFlags"]
}
}),
json(),
sourcemaps(),
nodeGlobals(),
nodeBuiltinsPlugin()
//viz({ filename: "dist-test/browser-stats.html", sourcemap: true })
],
onwarn: makeOnWarnForTesting(),
// Disable tree-shaking of test code. In [email protected], rollup started respecting
// the "sideEffects" field in package.json. Since our package.json sets "sideEffects=false", this also
// applies to test code, which causes all tests to be removed by tree-shaking.
treeshake: false
};

// (config.external as string[]).push(...Object.keys(pkg.devDependencies));

return config;
}

export function makeConfig(pkg: PackageJson) {
const baseConfig = {
input: "dist-esm/src/index.js",
external: [...nodeBuiltins, ...Object.keys(pkg.dependencies), ...Object.keys(pkg.devDependencies)],
output: { file: "dist/index.js", format: "cjs", sourcemap: true },
preserveSymlinks: false,
plugins: [sourcemaps(), nodeResolve(), cjs()]
}

if (IS_PRODUCTION) {
baseConfig.plugins.push(terser());
Copy link
Member

Choose a reason for hiding this comment

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

do we actually use/need this? Since we don't ship browser bundles I'm not sure why we'd want to bother minifying them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC @bterlson

}

const config: RollupOptions[] = [baseConfig as RollupOptions];

if (!IS_PRODUCTION) {
config.push(makeBrowserTestConfig());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is gated on the production env var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to me the logic was about whether or not we were building a production-ready distribution bundle, and in that case it seemed like we wouldn't need to generate the browser testing bundle, but honestly I don't know if/where we use NODE_ENV=production in the pipelines. I was expecting that I would see a CI failure if the browser test bundle was needed in any production contexts, but since I didn't...

I'll need to investigate this a little further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about it and I have come around to the same point of view on both this and the other IS_PRODUCTION comment. We don't need to do any minifying and we can just build the browser bundle by default. In any case, the NODE_ENV is the wrong switch to use here. I will instead add an options parameter with an opt-out flag for those packages that want to disable the browser test build.

}

return config;
}

4 changes: 2 additions & 2 deletions sdk/formrecognizer/ai-form-recognizer/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ module.exports = function(config) {

// list of files / patterns to load in the browser
files: [
"test-browser/index.js",
{ pattern: "test-browser/index.js.map", type: "html", included: false, served: true }
"dist-test/index.browser.js",
{ pattern: "dist-test/index.browser.js.map", type: "html", included: false, served: true }
].concat(isPlaybackMode() || isSoftRecordMode() ? ["recordings/browsers/**/*.json"] : []),

// list of files / patterns to exclude
Expand Down
19 changes: 5 additions & 14 deletions sdk/formrecognizer/ai-form-recognizer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"build:browser": "tsc -p . && cross-env ONLY_BROWSER=true rollup -c 2>&1",
"build:node": "tsc -p . && cross-env ONLY_NODE=true rollup -c 2>&1",
"build:samples": "dev-tool samples prep && cd dist-samples && tsc -p .",
"build:test": "tsc -p . && rollup -c rollup.test.config.js 2>&1",
"build:test": "tsc -p . && rollup -c 2>&1",
"build": "tsc -p . && rollup -c 2>&1 && api-extractor run --local",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
"clean": "rimraf dist dist-esm dist-browser dist-test test-browser temp types *.tgz *.log",
Expand All @@ -70,7 +70,7 @@
"test:node": "npm run build:test && npm run unit-test:node && npm run integration-test:node",
"test": "npm run build:test && npm run unit-test && npm run integration-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 120000 --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 1200000 --full-trace \"test/{,!(browser)/**/}*.spec.ts\"",
Copy link
Member

Choose a reason for hiding this comment

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

can we confirm that we still get good callstacks on failed tests?

Copy link
Member

Choose a reason for hiding this comment

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

also is the increase of timeout here necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

timeout shouldn't have been changed, that must be a bug. Error traces in console for both node and browser at least looked good but I'll try with vscode debugging to make sure that works as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deyaaeldeen actually, Form Recognizer was the only one with the lower timeout.

"unit-test": "npm run unit-test:node && npm run unit-test:browser"
},
"sideEffects": false,
Expand All @@ -92,12 +92,8 @@
"@azure/identity": "^1.1.0",
"@azure/test-utils-recorder": "^1.0.0",
"@microsoft/api-extractor": "7.7.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/fs-extra": "^8.0.0",
"@types/mocha": "^7.0.2",
"@types/node": "^8.0.0",
"@types/sinon": "^9.0.4",
Expand All @@ -112,6 +108,7 @@
"eslint-plugin-no-null": "^1.0.2",
"eslint-plugin-no-only-tests": "^2.3.0",
"eslint-plugin-promise": "^4.1.1",
"fs-extra": "^8.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

what's actually using fs-extra? I didn't see it in the base rollup config except as an external

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question. investigating why I did this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using it to create file streams for testing in Node.

"karma": "^5.1.0",
"karma-chrome-launcher": "^3.0.0",
"karma-coverage": "^2.0.0",
Expand All @@ -131,15 +128,9 @@
"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",
"typescript": "~3.9.3",
"fs-extra": "^8.1.0",
"@types/fs-extra": "^8.0.0"
"typescript": "~3.9.3"
},
"//smokeTestConfiguration": {
"skip": [
Expand Down
Loading