-
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
Conversation
function ignoreOpenTelemetryThisIsUndefinedWarnings(warning: RollupWarning): boolean { | ||
return ( | ||
warning.code === "THIS_IS_UNDEFINED" && warning.id?.includes("@opentelemetry/api") === 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.
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:
dist-esm/test/**/*.spec.js, dist-esm/test/**/node/** → dist-test/index.browser.js...
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en/#error-this-is-undefined
../../../common/temp/node_modules/.pnpm/@opentelemetry+api@1.0.3/node_modules/@opentelemetry/api/build/esm/api/context.js
14: * limitations under the License.
15: */
16: var __spreadArray = (this && this.__spreadArray) || function (to, from) {
^
17: for (var i = 0, il = from.length, j = to.length; i < il; i++, j++)
18: to[j] = from[i];
...and 1 other occurrence
../../../common/temp/node_modules/.pnpm/@opentelemetry+api@1.0.3/node_modules/@opentelemetry/api/build/esm/context/NoopContextManager.js
14: * limitations under the License.
15: */
16: var __spreadArray = (this && this.__spreadArray) || function (to, from) {
^
17: for (var i = 0, il = from.length, j = to.length; i < il; i++, j++)
18: to[j] = from[i];
...and 1 other occurrence
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.
@@ -23,7 +23,7 @@ describe("Certificates client's user agent (only in Node, because of fs)", () => | |||
try { | |||
// The unit-test script has this test file at: test/internal/userAgent.spec.ts | |||
const fileContents = JSON.parse( | |||
fs.readFileSync(path.join(__dirname, "../package.json"), { encoding: "utf-8" }) | |||
fs.readFileSync(path.join(__dirname, "../../package.json"), { encoding: "utf-8" }) |
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.
huh, I wonder why it was passing before.
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.
likely the bundled nodejs test goes through the catch
block below?
@@ -103,11 +111,15 @@ 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 comment
The 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 comment
The 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 comment
The 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 pkg
an optional param and default to dist-esm/src/index.js
if it's not provided like in https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/dev-tool/src/config/rollup.base.config.ts#L162
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 comment
The reason will be displayed to describe this comment to others. Learn more.
edit: looking at it again, I guess if base config uses a default value if pkg["module"] does not exist maybe I should too...
Sounds good to me! Thanks!
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.
Sounds sensible to me.
export function makeBrowserTestConfig(pkg: PackageJson): RollupOptions { | ||
// ./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 comment
The reason will be displayed to describe this comment to others. Learn more.
There are only two packages that don't have module
:
- core-asynciterator-polyfill
- mock-hub
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 comment
The 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.
[Hub Generated] Review request for Microsoft.Network to add version stable/2022-05-01 (Azure#19091) * Adds base for updating Microsoft.Network from version stable/2020-11-01 to version 2022-05-01 * Updates readme * Updates API version in new specs and examples * Add frontdoor waf Patch version 2022-05-01 * Adds suppression to readme * Adds suppression to readme * Fix format errors * Fix validation issues * Fix ModelValidation error * fix valiation errors * Adds suppression to readme * Fix valiation error * Fix lintdiff error * Fix lintdiff error * Fix lintdiff error * Fix code format * Fix DefaultErrorResponseSchema lintDiff * Fix PrettierCheck error * Change ErrorResponse schema * Update network.json * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * Adds suppression to readme * suppress LintDiff
What
<package>-common
shared-source patternthis has been rewritten to undefined
errors for OpenTelemetryassert
withchai
Why
We're migrating packages over to the shared config, and this handles Key Vault packages.
Key Vault is the first migrated package that includes the shared-source keyvault-common pattern, and as such
it does not conform to the file structure rollup config expects. So we need to teach it to be a little smarter.
Finally, OTel's codebase emits the spreadArray helper
var __spreadArray = (this && this.__spreadArray) || ...
whichcauses Rollup to be unhappy as the top-level
this
is undefined. In this case (couldn't resist) it's harmless andcan be suppressed.
Finally, we are removing assert entirely so it doesn't make sense to add it to the namedExports collection - instead
I am just replacing it with
chai
wherever I encounter it.Resolves #13410