feat(core): Explicitly allow undefined as value for authToken option #805
Conversation
Updates type definition for optional property to include explicit undefined, improving clarity and consistency in type annotations.
There was a problem hiding this comment.
Pull Request Overview
This PR clarifies the type definition for the authToken property by making the undefined type explicit in the union type.
- Updated the
authTokenproperty type from optional to explicitly includeundefinedin the union type
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Why is the test failing? It doesn't seem to be related to my commit. |
There was a problem hiding this comment.
Hey @aiktb sorry for the delayed review! I think this is a reasonable change and not API-breaking (as for a conventional TS setup nothing changes and for TS setups with explicit undefined, we only widen the type). So let's get this merged in.
I reran your tests but if it fails again would you mind rebasing to the latest EDIT: looks like this fails consistently and you're already on the currently latest main branch? Most likely this is some kind of test flake.main branch commit. Thing is, your change can't reasonably cause this so my next guess would be that this is happening in this PR because you're an external contributor.
(Fully aware that ideally we should apply this to all optional options but I don't have the bandwidth for this at the moment. If you or anyone else wants to have the same type change for other optional properties, feel free to open a PR).
|
I'm gonna merge this and optimistically hope that this passes on main. Otherwise we need to figure something out 😅 |
* Revert "feat(core): Explicitly allow `undefined` as value for `authToken` option (#805)" This reverts commit 73787df. * Revert "Revert "feat(core): Explicitly allow `undefined` as value for `authToken` option (#805)"" This reverts commit 4ac2dc2. * some debug logging * . * .. * ... * .... * use posix? * try rollup w/ posix * move input file into rollup dir * check and add * wtf * more logging * so __dirname is the problem? * try w/o __dirname * fileURLToPath * simplify * avoid posix * use move bundle.js into webpack dir * use basename * dirname, not basename * start ci? * delete per-dir bundle.js, use fileUrlToPath everywhere
|
Tests were fixed via #807 (the reason they failed here is that they should have failed in general but for some reason didn't do so. I suspect the Nx cache to be responsible. Will investigate further) |
| datasource | package | from | to | | ---------- | ------------------- | ----- | ----- | | npm | @sentry/vite-plugin | 4.3.0 | 4.4.0 | ## [v4.4.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#440) - feat(core): Explicitly allow `undefined` as value for `authToken` option ([#805](getsentry/sentry-javascript-bundler-plugins#805)) - fix(core): Strip query strings from asset paths ([#806](getsentry/sentry-javascript-bundler-plugins#806)) Work in this release was contributed by [@aiktb](https://github.com/aiktb). Thank you for your contribution!
| datasource | package | from | to | | ---------- | ------------------- | ----- | ----- | | npm | @sentry/vite-plugin | 4.3.0 | 4.4.0 | ## [v4.4.0](https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/HEAD/CHANGELOG.md#440) - feat(core): Explicitly allow `undefined` as value for `authToken` option ([#805](getsentry/sentry-javascript-bundler-plugins#805)) - fix(core): Strip query strings from asset paths ([#806](getsentry/sentry-javascript-bundler-plugins#806)) Work in this release was contributed by [@aiktb](https://github.com/aiktb). Thank you for your contribution!
I always use
"exactOptionalPropertyTypes": true,in TypeScript, soauthToken? stringandauthToken?: string | undefined;are not equivalent. In short, the former type will cause the following code to have a TypeScript error:Like this:
