-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[KeyVault] - Use shared rollup config #19091
Changes from all commits
9d8e697
0ee37b8
388793a
5837440
619fb08
6602111
9be2e81
fc8cd87
80f36df
b856ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import cjs from "@rollup/plugin-commonjs"; | |
import sourcemaps from "rollup-plugin-sourcemaps"; | ||
import multiEntry from "@rollup/plugin-multi-entry"; | ||
import json from "@rollup/plugin-json"; | ||
import * as path from "path"; | ||
|
||
import nodeBuiltins from "builtin-modules"; | ||
|
||
|
@@ -83,9 +84,16 @@ function ignoreChaiCircularDependencyWarnings(warning: RollupWarning): boolean { | |
); | ||
} | ||
|
||
function ignoreOpenTelemetryThisIsUndefinedWarnings(warning: RollupWarning): boolean { | ||
return ( | ||
warning.code === "THIS_IS_UNDEFINED" && warning.id?.includes("@opentelemetry/api") === true | ||
); | ||
} | ||
|
||
const warningInhibitors: Array<(warning: RollupWarning) => boolean> = [ | ||
ignoreChaiCircularDependencyWarnings, | ||
ignoreNiseSinonEvalWarnings | ||
ignoreNiseSinonEvalWarnings, | ||
ignoreOpenTelemetryThisIsUndefinedWarnings | ||
]; | ||
|
||
/** | ||
|
@@ -103,11 +111,16 @@ function makeOnWarnForTesting(): (warning: RollupWarning, warn: WarningHandler) | |
|
||
// #endregion | ||
|
||
export function makeBrowserTestConfig(): RollupOptions { | ||
export function makeBrowserTestConfig(pkg: PackageJson): RollupOptions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is exported, could you update all calls to it? there is one call in schema registry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops! good call, thanks 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated it at the call because that's the only one I found and seemed to be a one-off case - but I was debating whether to make edit: looking at it again, I guess if base config uses a default value if pkg["module"] does not exist maybe I should too... What do you all think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! Thanks! |
||
// ./dist-esm/src/index.js -> ./dist-esm | ||
// ./dist-esm/keyvault-keys/src/index.js -> ./dist-esm/keyvault-keys | ||
const module = pkg["module"] ?? "dist-esm/src/index.js"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are only two packages that don't have
The rest that don't have it are perf-tests and samples. This default was really from a different time and I'd be fine with throwing an error in this case now assuming it doesn't break anything unexpected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only way I can see this logic breaking is if the index module isn't in the same place relative to the test folder in any package. That is something that might be worth just spot-checking to make sure we don't break a random package with this change. |
||
const basePath = path.dirname(path.parse(module).dir); | ||
|
||
const config: RollupOptions = { | ||
input: { | ||
include: ["dist-esm/test/**/*.spec.js"], | ||
exclude: ["dist-esm/test/**/node/**"] | ||
include: [path.join(basePath, "test", "**", "*.spec.js")], | ||
exclude: [path.join(basePath, "test", "**", "node", "**")] | ||
}, | ||
output: { | ||
file: `dist-test/index.browser.js`, | ||
|
@@ -173,7 +186,7 @@ export function makeConfig(pkg: PackageJson, options?: Partial<ConfigurationOpti | |
const config: RollupOptions[] = [baseConfig as RollupOptions]; | ||
|
||
if (!options.disableBrowserBundle) { | ||
config.push(makeBrowserTestConfig()); | ||
config.push(makeBrowserTestConfig(pkg)); | ||
} | ||
|
||
return config; | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
import * as base from "./rollup.base.config"; | ||
import { makeConfig } from "@azure/dev-tool/shared-config/rollup"; | ||
|
||
export default [base.nodeConfig()]; | ||
export default makeConfig(require("./package.json")); |
This file was deleted.
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.
is this warning the same one that was handled by
openTelemetryCommonJs
?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 a different one:
In this case
this
is undefined is fine, since the tslib helper that's emitted (At least that's what I think is happening here - correct me if I'm wrong) is valid JS and won't cause runtime issues.I wonder why it's being emitted from OTel - but given we are going to be removing OTel as a dependency of our SDK I don't know if it's worth investigating further - what do you think?
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 think your synopsis is correct and that we can suppress this warning. It's a warning because sometimes it's a problem and sometimes it's not. In this case it's not because top-level
this
is just part of the polyfill of array spreading in tslib.It must choose to emit this helper because the OTel compile target is not ES6 or higher. This is a little troubling because it likely means that we are not picking up the
module
field of the OTel API package. Their CommonJS files are ES5, but ESMs are ES6.