[Storage] Rollup for tests in storage #2297

HarshaNalluru · 2019-04-16T22:45:26Z

Update rollup-config as per the template.
Rollup for both the node and browser tests
Updated test:node command to use the generated dist-test/index.node.js instead of running ts-node on the test files.

"test:node": "cross-env TS_NODE_COMPILER_OPTIONS="{\"module\": \"commonjs\"}" nyc mocha --compilers ts-node/register --require source-map-support/register --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json --full-trace --no-timeouts test/*.test.ts test/node/*.test.ts",

updated to

"test:node": "mocha --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json -t 120000 dist-test/index.node.js",

Minor changes to rollup.base.config.js to get rollup working.
Restructured commands in package.json.
Renamed test files for consistency across packages.
Updated .nycrc to get code coverage with the updated version of nyc(14.0.0).

jeremymeng · 2019-04-17T00:42:01Z

sdk/storage/storage-blob/gulpfile.js

-      "browser/azure-storage.blob.min.js",
-      "browser/*.txt"
-    ])
+    .src(["browser/index.js", "browser/*.txt"])


I believe we still want the minified version. The template uses the uglify rollup plugin to do that.

[ Offline discussion with Jeremy ]

As per the template, the generated file is - browser/index.js (minified version)

This PR uses rollup-plugin-uglify to generate the minified version ("browser/index.js") just like the template does

Not sure if we should generate both the files
["browser/azure-storage.blob.js", "browser/azure-storage.blob.min.js" ]
the way storage-blob previously does

Still have to decide on the names for generated outputs!

My thoughts here: the standalone unminified version seems not important for npm package scenarios, but is possibly important when downloading a .zip release from e.g. GitHub. If we go the GH release route, we should have an unminified version, but otherwise I'm fine leaving it out. I'll also note that the unminified code will likely not be super nice to debug as it will still be bundled and have a bunch of "plumbing" goop.

In terms of file names we discussed on teams, how about we go with the following rule: the bundle file name is the package name with @ removed, / replaced with -, and suffixed with .min if minified. So azure-storage-blob.min.js.

"Modify template to generate both minified and unminified versions of src code through rollup and update the names of outputs"
Issue to track - #2416

Both js and min.js are needed in the generated zip bundle to be published. See https://aka.ms/downloadazurestoragejsblob

Yes @XiaoningLiu, we are doing it now.
Now, the generated zip bundle contains both "azure-storage-blob.min.js" and "azure-storage-blob.js"

jeremymeng

Please work on adding uglify.

Also I am not sure whether we should keep the old file names for browser. @bterlson what's recommended for naming js files for browser?

bterlson · 2019-04-19T00:28:57Z

sdk/storage/storage-blob/gulpfile.js

-      "browser/azure-storage.blob.min.js",
-      "browser/*.txt"
-    ])
+    .src(["browser/index.js", "browser/*.txt"])


My thoughts here: the standalone unminified version seems not important for npm package scenarios, but is possibly important when downloading a .zip release from e.g. GitHub. If we go the GH release route, we should have an unminified version, but otherwise I'm fine leaving it out. I'll also note that the unminified code will likely not be super nice to debug as it will still be bundled and have a bunch of "plumbing" goop.

In terms of file names we discussed on teams, how about we go with the following rule: the bundle file name is the package name with @ removed, / replaced with -, and suffixed with .min if minified. So azure-storage-blob.min.js.

bterlson · 2019-04-19T00:30:20Z

sdk/storage/storage-blob/rollup.base.config.js

+        return;
+      }
+
+      if (


Do you need this custom onwarn machinery? It's included in the template as an example, but if it's not needed here feel free to remove it.

I remember facing an issue with "this" which got resolved when I added this(for browser)
I'll verify once again.

**Removed chai warning block(not required).
"THIS_IS_UNDEFINED" is required

bterlson · 2019-04-19T00:31:14Z

sdk/storage/storage-blob/rollup.base.config.js

+      file: "browser/index.js",
+      banner: banner,
+      format: "umd",
+      name: "azblob",


Just a note: this will likely change to align with .net naming conventions. Expect something like Azure.Storage.Blob instead, but let's keep it the same for now.

Please don't change this before next breaking change. : )

bterlson · 2019-04-19T00:32:30Z

sdk/storage/storage-blob/rollup.base.config.js

+        }
+      ),
+      // os is not used by the browser bundle, so just shim it
+      shim({


Are these shims actually needed?

dotenv is needed

os is present from the beginning

Update - shimming both of them is required.

bterlson · 2019-04-23T00:30:43Z

sdk/storage/storage-blob/rollup.base.config.js

+    plugins: [
+      sourcemaps(),
+      replace(
+        // ms-rest-js is externalized so users must include it prior to using this bundle.


I'm not sure this comment is relevant and can be deleted IMO.

XiaoningLiu · 2019-05-07T04:14:00Z

sdk/storage/storage-blob/gulpfile.js

@@ -7,8 +7,8 @@ const zipFileName = `azurestoragejs.blob-${version}.zip`;
 gulp.task("zip", function(callback) {
  gulp
    .src([
-      "browser/azure-storage.blob.js",
-      "browser/azure-storage.blob.min.js",
+      "browser/azure-storage-blob.min.js",


Please update section "JavaScript Bundle" in "readme.md". Same with other readme.md files for queue/file. Please check any documents points to old bundle name.

Thanks for pointing out.
Done - 7788fd0

XiaoningLiu · 2019-05-07T04:14:53Z

sdk/storage/storage-blob/package.json

@@ -82,7 +83,7 @@
    "prebuild": "npm run clean",
    "pretest": "npm run build:test",
    "test:browser": "karma start --single-run",
-    "test:node": "cross-env TS_NODE_COMPILER_OPTIONS=\"{\\\"module\\\": \\\"commonjs\\\"}\" nyc mocha --compilers ts-node/register --require source-map-support/register --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json --full-trace --no-timeouts test/*.test.ts test/node/*.test.ts",
+    "test:node": "nyc mocha --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json -t 120000 dist-test/index.node.js",


How does coverage report look like? Do covered lines point to TS source code or compiled JS codes?

Addressed in the other thread.
#2297 (comment)

XiaoningLiu · 2019-05-07T04:15:53Z

sdk/storage/storage-blob/rollup.base.config.js

+      sourcemap: true
+    },
+    plugins: [
+      sourcemaps(),


What does "sourcemaps()" do? I remembered a sourcemap file is generated under dist folder without this plugin.

This plugin helps in grabbing source maps from sourceMappingURLs.

For instance, look at the differences in nyc code coverage output.

with sourcemaps()

without sourcemaps()

Got it. Can you remove "dotenv" from coverage report?

sourcemaps is for threading the sourcemap across multiple transformations. Rollup by default won't consume sourcemaps so the dist sourcemaps would be a mapping to dist-esm rather than the original TS code.

Got it. Can you remove "dotenv" from coverage report?

Update -

I tried excluding that specific file(and every possible node_modules path) by adding rules in .nycrc file and could not exclude this file in any way.

Looks like nyc is not properly honouring the exclude rule in .nycrc. I see some similar recent user issues in the nyc repo - istanbuljs/nyc#1099

I'm opening a new issue to investigate further - #2820

@HarshaNalluru - I apologize. The issue 1099 that you reference was mine. It was user error.

XiaoningLiu · 2019-05-07T04:19:08Z

sdk/storage/storage-blob/rollup.base.config.js

+        }
+      }),
+      nodeResolve({ preferBuiltins: true }),
+      cjs(),


Is"common-js" plugin necessary? Previously we didn't have this.

cjs() converts CommonJS modules to ES6, so they can be included in a Rollup bundle.

This is required for proper functioning of the dotenv module.

dotenv module has been added to the storage packages as a devDependency - [Storage] Environment variables for storage sdk #1610. This package helps to load the environment variables from a .env file instead of setting them each time we run the tests.

Example use case

azure-sdk-for-js/sdk/storage/storage-blob/test/aborter.test.ts

Lines 5 to 9 in 55ba1e0

import { getBSU, getUniqueName } from "./utils";

import * as dotenv from "dotenv";

dotenv.config({path:"../.env"});

// tslint:disable:no-empty

XiaoningLiu · 2019-05-07T04:19:53Z

sdk/storage/storage-blob/rollup.base.config.js

+      }),
+      nodeResolve({ preferBuiltins: true }),
+      cjs(),
+      json()


Is "json" plugin necessary?

From my understanding, rollup-plugin-json is not necessary.
Removed it - 518a1f0

XiaoningLiu · 2019-05-07T04:25:31Z

sdk/storage/storage-blob/rollup.base.config.js

+      console.error(`(!) ${warning.message}`);
+    };
+  } else if (production) {
+    baseConfig.plugins.push(uglify());


For Node.js, we released minified CJS bundle to customers. When there is an exception in SDK, stack trace is unreadable. Any good suggestions? @bterlson @jeremymeng @HarshaNalluru

Sourcemaps! See below.

XiaoningLiu · 2019-05-07T04:27:16Z

sdk/storage/storage-blob/package.json

@@ -82,7 +83,7 @@
    "prebuild": "npm run clean",
    "pretest": "npm run build:test",
    "test:browser": "karma start --single-run",
-    "test:node": "cross-env TS_NODE_COMPILER_OPTIONS=\"{\\\"module\\\": \\\"commonjs\\\"}\" nyc mocha --compilers ts-node/register --require source-map-support/register --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json --full-trace --no-timeouts test/*.test.ts test/node/*.test.ts",
+    "test:node": "nyc mocha --reporter mocha-multi-reporters --reporter-options configFile=mocha.reporter.config.json -t 120000 dist-test/index.node.js",


When we test against a single bundle file, failed case reported stack trace is not friendly (not pointing to original TS files), makes it difficulty to locate to specific line for debugging. Especially to debug failed cases reported in CIs. Any good suggestions?

That's a good question. But currently, we don't have a way.
(Even for other packages, reported stack trace doesn't point to the original TS files).
I can start a new issue to track this.

[On a side note, a single bundle file would help us enable VS Code debugging effortlessly.]

Aha, how did you debug in VS code with a single bundle file? will break point work properly? I previously use node-ts, and it works great especial for break points debugging.

It will point to the source files(typescript) using the source mappings.
We can set breakpoints in the typescript source files.

Created a draft PR for VS Code Debugging -
https://github.com/Azure/azure-sdk-for-js/pull/2770/files#diff-8b42a0e706358bbff6b0a06ac092e5ac.

Once this setup is done, we would basically never have to update anything in the VS Code launch.json since the configuration will correspond to all the tests.

[Contains changes from this PR, can only be merged after this PR for rollup]

thanks, that's very useful!

Source maps with vscode are pretty sweet. But there is also this library we could investigate as a dev dependency: https://github.com/evanw/node-source-map-support. It maps all the file/offset data in exception payloads using sourcemaps.

Thanks Brian, creating a new issue to track this - #2792

XiaoningLiu · 2019-05-07T04:31:09Z

sdk/storage/storage-blob/rollup.base.config.js

+    // mark assert as external
+    baseConfig.external.push("assert", "fs", "path");
+
+    baseConfig.onwarn = warning => {


Interesting, for 10.1.0-preview, we have similar roll up config for Node.js test cases. However, doesn't have met this "THIS_IS_UNDEFINED" warning. Can you check why previous version doesn't have this issue?

See https://github.com/Azure/azure-storage-js/blob/v10.1.0-preview-blob/blob/rollup.test.config.js

It's possible this ignore was added because it was needed in other projects and isn't needed here. Harsha can confirm.

We do see "THIS_IS_UNDEFINED" warning which I could only get rid of using custom onwarn handlers.
Also, I've verified v10.1.0-preview branch, I need to look deeper on why it didn't throw those warnings previously.
I'm guessing this might be from version updates in the dependencies[not really sure].

Update -

The following line made the difference.
node.context = "null"
https://github.com/Azure/azure-storage-js/blob/v10.1.0-preview-blob/blob/rollup.test.config.js#L9

By commenting that line, I was able to reproduce the (!) 'this' has been rewritten to 'undefined' warnings with the code in v10.1.0-preview-blob branch(tag).

With the code in the current PR, and by leveraging this line of code(context = "null") instead of onwarn handler, I am still able to get rid of the warnings.

From the documentation https://rollupjs.org/guide/en#danger-zone, it is not clear to me on what context = "null" does.

Thanks @HarshaNalluru ! Here is the description about "THIS NOT DEFINED" in rollup doc. options.context is a workaround to override default value of this. @bterlson previously added node.context = "null" in v10.1.0-preview. I believe this approach should work.

Error: "this is undefined"
In a JavaScript module, this is undefined at the top level (i.e., outside functions). Because of that, Rollup will rewrite any this references to undefined so that the resulting behaviour matches what will happen when modules are natively supported.

There are occasional valid reasons for this to mean something else. If you're getting errors in your bundle, you can use options.context and options.moduleContext to change this behaviour.

Removed onwarn handlers and updated with context = "null"
2d2c526

XiaoningLiu

Generally looks good to me, thanks @HarshaNalluru ! 2 comments left

dotenv is included in the coverage repo, should be remove
supress-warning for undefined this, we'd better find out the root cause, if there any better workaround to fix the issue instead of ignore the warning

…e-sdk-for-js into RollupStorage

HarshaNalluru added 9 commits April 16, 2019 12:23

Rename test files for consistency

5663490

rollup for node tests

0fb1846

rollup for node tests

558cac6

this is undefined, update paths for rollup

e6803cb

Import dotenv in index.ts

9042ef6

Update output path in rollup

9471ef0

del mocha report conf - pass args in live test cmd

44570b2

restructure package.json similar to other pkgs

e72f5cd

remove commented code

154af83

HarshaNalluru requested review from bterlson and jeremymeng April 16, 2019 22:45

HarshaNalluru self-assigned this Apr 16, 2019

HarshaNalluru added Storage Storage Service (Queues, Blobs, Files) EngSys This issue is impacting the engineering system. labels Apr 16, 2019

Add mocha.reporter.config to publish node results

66a5fed

jeremymeng reviewed Apr 17, 2019

View reviewed changes

jeremymeng requested changes Apr 17, 2019

View reviewed changes

bterlson requested changes Apr 19, 2019

View reviewed changes

HarshaNalluru added 6 commits April 22, 2019 14:33

Merge remote-tracking branch 'upstream/master' into RollupStorage

6d2fc3f

Remove unwanted warnings, code for min version

6363eb4

Update gulp to zip both min and unmin versions

93e42fc

Update rollup to generate min and unmin versions

7bddadf

Add (commented) rollup visualizer

0a311f9

remove unused import path

4fecbcc

HarshaNalluru requested review from jeremymeng and bterlson April 23, 2019 00:06

bterlson approved these changes Apr 23, 2019

View reviewed changes

HarshaNalluru mentioned this pull request Apr 23, 2019

[Template] Update rollup for browser in the template #2416

Closed

HarshaNalluru requested a review from XiaoningLiu April 23, 2019 01:09

remove unnecessary comment

4eba327

Increase timeout to 120 seconds for tests

34e9fc5

This was referenced May 1, 2019

[Storage] Update live test command for storage packages #2616

Merged

[Service Bus] Fix NYC code coverage breaking changes in 14.0.0 #2627

Merged

XiaoningLiu requested changes May 7, 2019

View reviewed changes

HarshaNalluru force-pushed the RollupStorage branch from 56b4452 to 34e9fc5 Compare May 7, 2019 19:34

HarshaNalluru added 6 commits May 7, 2019 13:11

resolve merge conflicts

7d2b4ba

resolve merge conflicts

3237181

Update JavaScript Bundle file names

7788fd0

Update test path in karma conf

0e2d10d

Merge remote-tracking branch 'upstream/master' into RollupStorage

cf6524c

undo adding rollup-plugin-json for storage-pkgs

518a1f0

HarshaNalluru force-pushed the RollupStorage branch from ae9dc5b to 518a1f0 Compare May 8, 2019 21:04

HarshaNalluru and others added 3 commits May 8, 2019 14:29

resolve merge conflicts

3dd97f2

Update test command as the per updates to master

fb50e69

Merge branch 'master' into RollupStorage

ea3b017

HarshaNalluru requested a review from XiaoningLiu May 8, 2019 22:47

XiaoningLiu approved these changes May 9, 2019

View reviewed changes

HarshaNalluru mentioned this pull request May 9, 2019

[Storage] Source map support for stack traces #2792

Closed

HarshaNalluru added 2 commits May 10, 2019 15:48

Resolve merge conflicts

21f1302

Merge branch 'RollupStorage' of https://github.com/HarshaNalluru/azur…

10b71ee

…e-sdk-for-js into RollupStorage

HarshaNalluru mentioned this pull request May 11, 2019

[Storage] Exclude dotenv package from nyc code coverage #2820

Closed

HarshaNalluru and others added 3 commits May 13, 2019 00:53

remove onwarn handler, update with context="null"

2d2c526

Merge branch 'master' into RollupStorage

312b42b

Merge branch 'master' into RollupStorage

5a630c7

HarshaNalluru requested a review from XiaoningLiu May 14, 2019 21:13

jeremymeng approved these changes May 14, 2019

View reviewed changes

XiaoningLiu approved these changes May 15, 2019

View reviewed changes

Merge branch 'master' into RollupStorage

c7f913e

HarshaNalluru merged commit 58a0881 into Azure:master May 15, 2019

HarshaNalluru mentioned this pull request May 15, 2019

[Storage] Update rollup-config as per the template #2326

Closed

+                      return;
+                    }
+                    if (

	import { getBSU, getUniqueName } from "./utils";
	import * as dotenv from "dotenv";
	dotenv.config({path:"../.env"});

	// tslint:disable:no-empty

[Storage] Rollup for tests in storage #2297

[Storage] Rollup for tests in storage #2297

Conversation

HarshaNalluru commented Apr 16, 2019 • 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

XiaoningLiu Apr 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremymeng 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

HarshaNalluru Apr 19, 2019 • 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

HarshaNalluru May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru May 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru May 8, 2019 • 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

HarshaNalluru May 8, 2019 • 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

XiaoningLiu left a comment

Choose a reason for hiding this comment

HarshaNalluru commented Apr 16, 2019 •

edited

Loading

XiaoningLiu Apr 23, 2019 •

edited

Loading

HarshaNalluru Apr 19, 2019 •

edited

Loading

HarshaNalluru May 8, 2019 •

edited

Loading

HarshaNalluru May 11, 2019 •

edited

Loading

HarshaNalluru May 8, 2019 •

edited

Loading

HarshaNalluru May 8, 2019 •

edited

Loading

HarshaNalluru May 8, 2019 •

edited

Loading