[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,185 @@ | ||
| // 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"; | ||
|
|
||
| import path from "path"; | ||
|
|
||
| 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(), | ||
|
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(), | ||
|
There was a problem hiding this comment. Is"common-js" plugin necessary? Previously we didn't have this. There was a problem hiding this comment.
|
||
| json() | ||
|
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 => { | ||
|
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; | ||
| } | ||
|
|
||
| if ( | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. I remember facing an issue with "this" which got resolved when I added this(for browser) There was a problem hiding this comment. **Removed chai warning block(not required). |
||
| warning.code === "CIRCULAR_DEPENDENCY" && | ||
| warning.importer.indexOf(path.normalize("node_modules/chai/lib") === 0) | ||
| ) { | ||
| // Chai contains circular references, but they are not fatal and can be ignored. | ||
| 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) { | ||
| const baseConfig = { | ||
| input: "dist-esm/src/index.browser.js", | ||
| external: ["ms-rest-js"], | ||
| output: { | ||
| file: "browser/index.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 | ||
| }, | ||
| plugins: [ | ||
| sourcemaps(), | ||
| replace( | ||
| // ms-rest-js is externalized so users must include it prior to using this bundle. | ||
|
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({ | ||
|
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({ | ||
| 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; | ||
| } | ||
|
|
||
| if ( | ||
| warning.code === "CIRCULAR_DEPENDENCY" && | ||
| warning.importer.indexOf(path.normalize("node_modules/chai/lib") === 0) | ||
| ) { | ||
| // Chai contains circular references, but they are not fatal and can be ignored. | ||
| return; | ||
| } | ||
|
|
||
| console.error(`(!) ${warning.message}`); | ||
| }; | ||
| } else if (production) { | ||
| baseConfig.plugins.push(uglify()); | ||
| } | ||
|
|
||
| return baseConfig; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,101 +1,17 @@ | ||
| // 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()); | ||
| } | ||
|
|
||
| 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.
I believe we still want the minified version. The template uses the
uglifyrollup plugin to do that.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.
[ Offline discussion with Jeremy ]
browser/index.js(minified version)rollup-plugin-uglifyto generate the minified version ("browser/index.js") just like the template does["browser/azure-storage.blob.js", "browser/azure-storage.blob.min.js" ]
the way storage-blob previously does
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.
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
.minif minified. Soazure-storage-blob.min.js.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.
"Modify template to generate both minified and unminified versions of src code through rollup and update the names of outputs"
Issue to track - #2416
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.
Both js and min.js are needed in the generated zip bundle to be published. See https://aka.ms/downloadazurestoragejsblob
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.
Yes @XiaoningLiu, we are doing it now.
Now, the generated zip bundle contains both "azure-storage-blob.min.js" and "azure-storage-blob.js"