[dev-tool] Shared rollup config factory #10923

willmtemple · 2020-08-29T00:00:01Z

This PR is a first stab at creating a universal rollup configuration for our libraries. It migrates:

template
ai-form-recognizer
ai-text-analytics

There are no warnings left on prod or testing builds, and there are some included changes to how tests are run:

dev-tool now holds a shared-config folder where shared configuration can live. This is a small shim program that loads a strongly-typed version of the config in src/config/rollup.base.config.ts.
- Load the shim by requiring @azure/dev-tool/shared-config/rollup.
- Pass the package JSON object to the makeConfig function, see rollup.config.js below.
There is now only one configuration that builds both the release rollup as well as the browser test build.
Node tests are now run using ts-node, and are no longer bundled.
All node builtins, package dependencies, and dev dependencies are now excluded from the rollup bundle by default (no more messing with "excludes". In browser test mode, everything is bundled together.
Node globals and builtins are automatically shimmed in the browser as appropriate.
Known inconsequential warnings (such as circular dependency in chai and use of eval in sinon and nise) are automatically inhibited in the test configuration.
The build generates source maps (as it always has), and karma is set up to load them. Node test runs get source map support automatically through ts-node.

Notes and next steps:

Most libraries should remove instances of isNode, and prefer instead to use browser mappings where appropriate, but since we're not shipping browser bundles.
If this approach is agreeable, I'll start looking at Azure core.

xirzec

Overall, I love the direction of this PR as not having to copy-paste confusing/massive configuration files is a breath of fresh air.

I left some minor comments, the oddest being the existence of files with strange names that I think got committed as part of some buffer?

common/tools/dev-tool/'

xirzec · 2020-09-02T21:40:59Z

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

+  }
+
+  if (IS_PRODUCTION) {
+    baseConfig.plugins.push(terser());


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

CC @bterlson

xirzec · 2020-09-02T21:41:51Z

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

+  const config: RollupOptions[] = [baseConfig as RollupOptions];
+
+  if (!IS_PRODUCTION) {
+    config.push(makeBrowserTestConfig());


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

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.

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.

xirzec · 2020-09-02T21:42:32Z

sdk/formrecognizer/ai-form-recognizer/package.json

@@ -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\"",


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

also is the increase of timeout here necessary?

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.

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

xirzec · 2020-09-02T21:43:52Z

sdk/formrecognizer/ai-form-recognizer/package.json

@@ -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",


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

good question. investigating why I did this.

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

sdk/template/template/package.json

sdk/textanalytics/ai-text-analytics/]

deyaaeldeen

Awesome! Thanks for doing this. Looks great except for the weird files checked in. IIRC, last time I used vscode debugger with ts-node, it worked well. Can you verify it works in this PR?

deyaaeldeen · 2020-09-03T14:16:28Z

sdk/formrecognizer/ai-form-recognizer/package.json

@@ -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\"",


also is the increase of timeout here necessary?

deyaaeldeen · 2020-09-21T18:15:13Z

sdk/formrecognizer/ai-form-recognizer/package.json

@@ -108,7 +107,6 @@
    "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",


deyaaeldeen · 2020-09-21T18:19:44Z

sdk/template/template/package.json

    "typescript": "~3.9.3",
-    "util": "^0.12.1"
+    "util": "^0.12.1",
+    "karma-typescript": "~5.0.3"


is this needed? we do not have it in TA.

This is a leftover from when I was playing with this plugin in the karma config, thanks for pointing this out!

deyaaeldeen

Looks great! I left one minor comment.

xirzec

Changes look good! Thanks again for driving this. 👍

Add some profile tag for go SDK to serve the new profile 2020-09-01 (Azure#10923) * some update of the tags for profiles * final update for profiles * a fix of tag name * Remove the changes for web readme since it does not work * Rename tag for compute, revert keyvault tag change * update

witemple-msft added 3 commits August 10, 2020 10:58

WIP

81d1203

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

685f8d1

[dev-tool] Abstract rollup config factory

015266f

willmtemple added Client This issue points to a problem in the data-plane of the library. dev-tool Issues related to the Azure SDK for JS dev-tool labels Aug 29, 2020

willmtemple requested review from bterlson, deyaaeldeen, jeremymeng, KarishmaGhiya, mikeharder, ramya-rao-a and xirzec as code owners August 29, 2020 00:00

fix typo

25af0f1

willmtemple mentioned this pull request Aug 31, 2020

[Storage] Fix rollup warnings #3993

Closed

witemple-msft added 5 commits August 31, 2020 09:24

Fixed typeck error

33889bf

dependency cleanup

9dffc29

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

ac2edcb

Removed errant test config from template

ff11f10

Fixed template karma configuration

3b89784

xirzec reviewed Sep 2, 2020

View reviewed changes

deyaaeldeen reviewed Sep 3, 2020

View reviewed changes

deyaaeldeen mentioned this pull request Sep 11, 2020

[Key Vault Admin] Tests #11010

Merged

Removed ghost files, small tweak to base config

d19da79

willmtemple requested a review from a team as a code owner September 12, 2020 03:32

witemple-msft added 6 commits September 14, 2020 10:48

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

6b5b4bb

Removed explicit config env from template package.json

641441b

Removed IS_PRODUCTION gates

8b79559

Removed dependency on terser

c6b182d

Removed dependency on fs-extra

0c96ff2

Added flag to disable browser test bundle

958e67a

witemple-msft added 2 commits September 16, 2020 09:17

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

65839dc

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

c737534

willmtemple requested review from xirzec and deyaaeldeen September 18, 2020 21:08

deyaaeldeen reviewed Sep 21, 2020

View reviewed changes

deyaaeldeen approved these changes Sep 21, 2020

View reviewed changes

witemple-msft added 2 commits September 24, 2020 12:50

Removed karma-typescript

9820cef

Merge remote-tracking branch 'upstream/master' into rollup-abstraction

3e9bf49

xirzec approved these changes Sep 25, 2020

View reviewed changes

willmtemple merged commit 8bb505f into Azure:master Sep 28, 2020

maorleger mentioned this pull request Jul 26, 2021

KeyVault - use shared rollup config from dev-tool #13410

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dev-tool] Shared rollup config factory #10923

[dev-tool] Shared rollup config factory #10923

willmtemple commented Aug 29, 2020

xirzec left a comment

xirzec Sep 2, 2020

willmtemple Sep 8, 2020

xirzec Sep 2, 2020

willmtemple Sep 8, 2020

willmtemple Sep 15, 2020

xirzec Sep 2, 2020

deyaaeldeen Sep 3, 2020

willmtemple Sep 8, 2020

willmtemple Sep 15, 2020

xirzec Sep 2, 2020

willmtemple Sep 8, 2020

willmtemple Sep 14, 2020

deyaaeldeen left a comment

deyaaeldeen Sep 3, 2020

deyaaeldeen Sep 21, 2020

deyaaeldeen Sep 21, 2020

willmtemple Sep 24, 2020

deyaaeldeen left a comment •

edited

Loading

xirzec left a comment

[dev-tool] Shared rollup config factory #10923

[dev-tool] Shared rollup config factory #10923

Conversation

willmtemple commented Aug 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

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

deyaaeldeen 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

deyaaeldeen left a comment • edited Loading

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

deyaaeldeen left a comment •

edited

Loading