[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 @@ | ||
| { | ||
| "reporterEnabled": "spec, mocha-junit-reporter", | ||
| "mochaJunitReporterReporterOptions": { | ||
| "mochaFile": "test-results.xml" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,179 @@ | ||
| // 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 json from "rollup-plugin-json"; | ||
| 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 | ||
| }, | ||
| plugins: [ | ||
| sourcemaps(), | ||
|
Member
There was a problem hiding this comment. What does "sourcemaps()" do? I remembered a sourcemap file is generated under dist folder without this plugin.
Contributor
Author
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.
Member
There was a problem hiding this comment. Got it. Can you remove "dotenv" from coverage report?
Member
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
Contributor
Author
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(), | ||
|
Member
There was a problem hiding this comment. Is"common-js" plugin necessary? Previously we didn't have this.
Contributor
Author
There was a problem hiding this comment.
|
||
| json() | ||
|
Contributor
Author
There was a problem hiding this comment. From my understanding, |
||
| ] | ||
| }; | ||
|
|
||
| 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 => { | ||
|
Member
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?
Member
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.
Contributor
Author
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.
Contributor
Author
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
Member
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.
Contributor
Author
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()); | ||
|
Member
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", | ||
|
Member
There was a problem hiding this comment. Just a note: this will likely change to align with .net naming conventions. Expect something like
Member
There was a problem hiding this comment. Please don't change this before next breaking change. : ) |
||
| sourcemap: true | ||
| }, | ||
| plugins: [ | ||
| sourcemaps(), | ||
| replace( | ||
| // ms-rest-js is externalized so users must include it prior to using this bundle. | ||
|
Member
There was a problem hiding this comment. I'm not sure this comment is relevant and can be deleted IMO. |
||
| { | ||
| 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({ | ||
|
Member
There was a problem hiding this comment. Are these shims actually needed?
Contributor
Author
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({ | ||
| module: true, | ||
| browser: true, | ||
| preferBuiltins: false | ||
| }), | ||
| cjs({ | ||
| namedExports: { | ||
| events: ["EventEmitter"], | ||
| assert: [ | ||
| "ok", | ||
| "deepEqual", | ||
| "equal", | ||
| "fail", | ||
| "deepStrictEqual", | ||
| "notDeepEqual" | ||
| ] | ||
| } | ||
| }), | ||
| json() | ||
| ] | ||
| }; | ||
|
|
||
| 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,101 +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; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,6 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import * as base from "./rollup.base.config"; | ||
|
|
||
| export default [base.nodeConfig(true), base.browserConfig(true)]; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out.
Done - 7788fd0