[Storage] Rollup for tests in storage #2297
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| # browser # | ||
| browser/azure-storage-blob.js | ||
| browser/azure-storage-blob.js.map | ||
|
|
||
| # dist-test # | ||
| dist-test/ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,8 @@ const zipFileName = `azurestoragejs.blob-${version}.zip`; | |
| gulp.task("zip", function(callback) { | ||
| gulp | ||
| .src([ | ||
| "browser/azure-storage-blob.min.js", | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. Thanks for pointing out. |
||
| "browser/azure-storage-blob.js", | ||
| "browser/*.txt" | ||
| ]) | ||
| .pipe(zip(zipFileName)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import nodeResolve from "rollup-plugin-node-resolve"; | ||
| import multiEntry from "rollup-plugin-multi-entry"; | ||
| import cjs from "rollup-plugin-commonjs"; | ||
| import replace from "rollup-plugin-replace"; | ||
| import { uglify } from "rollup-plugin-uglify"; | ||
| import sourcemaps from "rollup-plugin-sourcemaps"; | ||
| import shim from "rollup-plugin-shim"; | ||
| // import visualizer from "rollup-plugin-visualizer"; | ||
|
|
||
| const version = require("./package.json").version; | ||
| const banner = [ | ||
| "/*!", | ||
| ` * Azure Storage SDK for JavaScript - Blob, ${version}`, | ||
| " * Copyright (c) Microsoft and contributors. All rights reserved.", | ||
| " */" | ||
| ].join("\n"); | ||
|
|
||
| const pkg = require("./package.json"); | ||
| const depNames = Object.keys(pkg.dependencies); | ||
| const production = process.env.NODE_ENV === "production"; | ||
|
|
||
| export function nodeConfig(test = false) { | ||
| const externalNodeBuiltins = [ | ||
| "@azure/ms-rest-js", | ||
| "crypto", | ||
| "fs", | ||
| "events", | ||
| "os", | ||
| "stream" | ||
| ]; | ||
| const baseConfig = { | ||
| input: "dist-esm/src/index.js", | ||
| external: depNames.concat(externalNodeBuiltins), | ||
| output: { | ||
| file: "dist/index.js", | ||
| format: "cjs", | ||
| sourcemap: true | ||
| }, | ||
| preserveSymlinks: false, | ||
| plugins: [ | ||
| sourcemaps(), | ||
|
There was a problem hiding this comment. What does "sourcemaps()" do? I remembered a sourcemap file is generated under dist folder without this plugin. There was a problem hiding this comment. This plugin helps in grabbing source maps from sourceMappingURLs. For instance, look at the differences in nyc code coverage output.
There was a problem hiding this comment. Got it. Can you remove "dotenv" from coverage report? There was a problem hiding this comment. sourcemaps is for threading the sourcemap across multiple transformations. Rollup by default won't consume sourcemaps so the There was a problem hiding this comment.
Update - I tried excluding that specific file(and every possible node_modules path) by adding rules in Looks like I'm opening a new issue to investigate further - #2820 There was a problem hiding this comment. @HarshaNalluru - I apologize. The issue 1099 that you reference was mine. It was user error. |
||
| replace({ | ||
| delimiters: ["", ""], | ||
| values: { | ||
| // replace dynamic checks with if (true) since this is for node only. | ||
| // Allows rollup's dead code elimination to be more aggressive. | ||
| "if (isNode)": "if (true)" | ||
| } | ||
| }), | ||
| nodeResolve({ preferBuiltins: true }), | ||
| cjs() | ||
| ] | ||
| }; | ||
|
|
||
| if (test) { | ||
| // entry point is every test file | ||
| baseConfig.input = [ | ||
| "dist-esm/test/*.spec.js", | ||
| "dist-esm/test/node/*.spec.js" | ||
| ]; | ||
| baseConfig.plugins.unshift(multiEntry({ exports: false })); | ||
|
|
||
| // different output file | ||
| baseConfig.output.file = "dist-test/index.node.js"; | ||
|
|
||
| // mark assert as external | ||
| baseConfig.external.push("assert", "fs", "path"); | ||
|
|
||
| baseConfig.onwarn = warning => { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. It's possible this ignore was added because it was needed in other projects and isn't needed here. Harsha can confirm. There was a problem hiding this comment. We do see "THIS_IS_UNDEFINED" warning which I could only get rid of using custom onwarn handlers. There was a problem hiding this comment. Update -
From the documentation https://rollupjs.org/guide/en#danger-zone, it is not clear to me on what There was a problem hiding this comment. Thanks @HarshaNalluru ! Here is the description about "THIS NOT DEFINED" in rollup doc. Error: "this is undefined" 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. There was a problem hiding this comment. Removed onwarn handlers and updated with |
||
| if (warning.code === "THIS_IS_UNDEFINED") { | ||
| // This error happens frequently due to TypeScript emitting `this` at the | ||
| // top-level of a module. In this case its fine if it gets rewritten to | ||
| // undefined, so ignore this error. | ||
| return; | ||
| } | ||
|
|
||
| console.error(`(!) ${warning.message}`); | ||
| }; | ||
| } else if (production) { | ||
| baseConfig.plugins.push(uglify()); | ||
|
There was a problem hiding this comment. 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 |
||
| } | ||
|
|
||
| return baseConfig; | ||
| } | ||
|
|
||
| export function browserConfig(test = false, production = false) { | ||
| const baseConfig = { | ||
| input: "dist-esm/src/index.browser.js", | ||
| external: ["ms-rest-js"], | ||
| output: { | ||
| file: "browser/azure-storage-blob.js", | ||
| banner: banner, | ||
| format: "umd", | ||
| name: "azblob", | ||
|
There was a problem hiding this comment. Just a note: this will likely change to align with .net naming conventions. Expect something like There was a problem hiding this comment. Please don't change this before next breaking change. : ) |
||
| sourcemap: true | ||
| }, | ||
| preserveSymlinks: false, | ||
| plugins: [ | ||
| sourcemaps(), | ||
| replace({ | ||
| delimiters: ["", ""], | ||
| values: { | ||
| // replace dynamic checks with if (false) since this is for | ||
| // browser only. Rollup's dead code elimination will remove | ||
| // any code guarded by if (isNode) { ... } | ||
| "if (isNode)": "if (false)" | ||
| } | ||
| }), | ||
| // os is not used by the browser bundle, so just shim it | ||
| shim({ | ||
|
There was a problem hiding this comment. Are these shims actually needed? There was a problem hiding this comment.
Update - shimming both of them is required. |
||
| dotenv: `export function config() { }`, | ||
| os: ` | ||
| export const type = 1; | ||
| export const release = 1; | ||
| ` | ||
| }), | ||
| nodeResolve({ | ||
| mainFields: ["module", "browser"], | ||
| preferBuiltins: false | ||
| }), | ||
| cjs({ | ||
| namedExports: { | ||
| events: ["EventEmitter"], | ||
| assert: [ | ||
| "ok", | ||
| "deepEqual", | ||
| "equal", | ||
| "fail", | ||
| "deepStrictEqual", | ||
| "notDeepEqual" | ||
| ] | ||
| } | ||
| }) | ||
| ] | ||
| }; | ||
|
|
||
| if (test) { | ||
| baseConfig.input = [ | ||
| "dist-esm/test/*.spec.js", | ||
| "dist-esm/test/browser/*.spec.js" | ||
| ]; | ||
| baseConfig.plugins.unshift(multiEntry({ exports: false })); | ||
| baseConfig.output.file = "dist-test/index.browser.js"; | ||
| baseConfig.onwarn = warning => { | ||
| if (warning.code === "THIS_IS_UNDEFINED") { | ||
| // This error happens frequently due to TypeScript emitting `this` at the | ||
| // top-level of a module. In this case its fine if it gets rewritten to | ||
| // undefined, so ignore this error. | ||
| return; | ||
| } | ||
|
|
||
| console.error(`(!) ${warning.message}`); | ||
| }; | ||
| } else if (production) { | ||
| baseConfig.output.file = "browser/azure-storage-blob.min.js"; | ||
| baseConfig.plugins.push( | ||
| uglify({ | ||
| output: { | ||
| preamble: banner | ||
| } | ||
| }) | ||
| // Comment visualizer because it only works on Node.js 8+; Uncomment it to get bundle analysis report | ||
| // visualizer({ | ||
| // filename: "./statistics.html", | ||
| // sourcemap: true | ||
| // }) | ||
| ); | ||
| } | ||
|
|
||
| return baseConfig; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,102 +1,18 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import * as base from "./rollup.base.config"; | ||
|
|
||
| const inputs = []; | ||
|
|
||
| if (!process.env.ONLY_BROWSER) { | ||
| inputs.push(base.nodeConfig()); | ||
| } | ||
|
|
||
| // Disable this until we are ready to run rollup for the browser. | ||
| if (!process.env.ONLY_NODE) { | ||
| inputs.push(base.browserConfig()); | ||
| inputs.push(base.browserConfig(false, true)); | ||
| } | ||
|
|
||
| export default inputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required since nyc is updated from 13.3.0 to 14.0.0.
[Breaking Change]
https://github.com/istanbuljs/nyc/blob/master/CHANGELOG.md
https://github.com/istanbuljs/nyc#source-map-support-for-pre-instrumented-codebases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, how does coverage report look like? Do covered lines point to TS source code or compiled JS codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Covered lines would point to the typescript source code.
Example -
Coverage output-html for (only) one test -