ref(types): Loosen tag types, create new Primitive type
#3108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
|
How about we create https://www.typescriptlang.org/docs/handbook/basic-types.html#object type Primititve = number | string | boolean | bigint | symbol | null | undefined;
type NonPrimitive = object; |
Oooh, I like it. Good idea. UPDATE: Done. |
lobsterkatie
commented
Dec 8, 2020
MIGRATION.md
Outdated
| `Scope`. | ||
|
|
||
| ```typescript | ||
| setTags(tags: { [key: string]: string }): this; | ||
| setTags(tags: { [key: string]: string | number | boolean | bigint| symbol| null | undefined }): this; |
There was a problem hiding this comment.
I listed them out here in the migration doc for ease of reading, but used the Primitive type everywhere in the code.
lobsterkatie
changed the title
Primitive type
lobsterkatie
force-pushed
the
kmclb-loosen-tag-types
branch
2 times, most recently
from
b833312 to
3d4c98f
Compare
HazAT
requested changes
Dec 15, 2020
| @@ -119,7 +119,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement | |||
| let eventId: string | undefined = hint && hint.event_id; | |||
|
|
|||
| const promisedEvent = isPrimitive(message) | |||
| ? this._getBackend().eventFromMessage(`${message}`, level, hint) | |||
| ? this._getBackend().eventFromMessage(String(message), level, hint) | |||
There was a problem hiding this comment.
Because symbols and bigints can't be automatically coerced to string form (see links in PR description) - you have to do it manually with String(x).
lobsterkatie
force-pushed
the
kmclb-loosen-tag-types
branch
from
6d06745 to
d1355bd
Compare
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Feb 22, 2021
One thing we'd like to have is the merged PR getsentry/sentry-javascript#3108, a type adjustment that lets us use `undefined` as a value in `setTag` and `setTags`. The author of that PR says that passing `undefined` for a tag's value is an unofficial way to unset a tag from the scope. If that works, we'd like to do it until getsentry/sentry-javascript#2218 is solved more officially. I see just one declared breaking change at 2.0.0 [1]: """ - build(android): Changes android package name from `io.sentry.RNSentryPackage` to `io.sentry.react.RNSentryPackage` (Breaking). zulip#1131 """ But autolinking handles the name change just fine, so there's nothing special we have to do about that. We don't have a complete libdef yet, but we seem to have good coverage for our usage in what we've translated from the TypeScript manually so far. So, double-check all of that against the new version and fix a few things that were slightly wrong or out-of-date (including by taking the relevant changes from getsentry/sentry-javascript#3108). [1] https://github.com/getsentry/sentry-react-native/blob/master/CHANGELOG.md#200
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Feb 22, 2021
…sent. Instead of omitting them. We're about to start keeping these tags on the global scope, which means we have to listen to and propagate all changes in the server version, including changes where the server version disappears. It's been reported [1] that passing `undefined` as a tag's value in `setTags` will clear the tag from the current scope, and I've verified this experimentally. [1] getsentry/sentry-javascript#3108 (comment)
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Feb 22, 2021
…sent. Instead of omitting them. We're about to start keeping these tags on the global scope, which means we have to listen to and propagate all changes in the server version, including changes where the server version disappears. It's been reported [1] that passing `undefined` as a tag's value in `setTags` will clear the tag from the current scope, and I've verified this experimentally. [1] getsentry/sentry-javascript#3108 (comment)
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only thing we do with tags is serialize/JSONify them, and any primitive* can handle that. And since we do that, relay isn't an issue, because all it ever sees from us is strings anyway.
This loosens the type requirements for tags to include all members of a new union type called
Primitive. It also adds a note to thesetTagdocstring pointing out that a tag can be unset by passingundefined. Finally, it refactorsisPrimitive()into a type guard, and uses that type guard where possible.Doesn't fully answer the request in #2218, but at least it will stop people's linters from freaking out when they pass
undefined, as we ourselves suggest.*Actually, symbols apparently don't let themselves be automatically cast to their string representation (EDIT: apparently BigInts don't either), so those are handled separately, in the
normalizeValuefunction.