-
Notifications
You must be signed in to change notification settings - Fork 557
feat(client-core-interfaces): JsonString
#25501
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
Changes from 1 commit
4da8ea2
3c7cb7f
324f5c3
95e3a42
4ef8371
64fbd43
d3906ba
fd5fb56
5cd751a
b890880
591c052
ee1d030
d636306
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,85 @@ | ||||||||||||||||||||||
| /*! | ||||||||||||||||||||||
| * Copyright (c) Microsoft Corporation and contributors. All rights reserved. | ||||||||||||||||||||||
| * Licensed under the MIT License. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- incorrect rule: misunderstands `declare`d types. | ||||||||||||||||||||||
| import type { BrandedType } from "./brandedType.js"; | ||||||||||||||||||||||
| import type { JsonDeserialized } from "./jsonDeserialized.js"; | ||||||||||||||||||||||
| import type { JsonSerializable } from "./jsonSerializable.js"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Brand for JSON that has been stringified. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * Usage: Intersect with another type to apply branding. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @sealed | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| declare class JsonStringBrand<T> extends BrandedType<JsonString<unknown>> { | ||||||||||||||||||||||
| public toString(): string; | ||||||||||||||||||||||
| protected readonly EncodedValue: T; | ||||||||||||||||||||||
| private constructor(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Branded `string` for JSON that has been stringified. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @remarks | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * Use {@link JsonStringify} to encode JSON producing values of this type and | ||||||||||||||||||||||
| * {@link JsonParse} to decode them. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * For custom encoding/decoding: | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * - cast to with `as unknown as JsonString<T>` when value of type `T` has been stringified. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * - use a form of {@link JsonDeserialized} for safety when parsing. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @sealed | ||||||||||||||||||||||
| * @internal | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export type JsonString<T> = string & JsonStringBrand<T>; | ||||||||||||||||||||||
CraigMacomber marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Options for {@link JsonStringify}. | ||||||||||||||||||||||
jason-ha marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * @internal | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| export interface JsonStringifyOptions { | ||||||||||||||||||||||
jason-ha marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * When set, inaccessible (protected and private) members throughout type T are | ||||||||||||||||||||||
| * ignored as if not present. Otherwise, inaccessible members are considered | ||||||||||||||||||||||
| * an error (type checking will mention `SerializationErrorPerNonPublicProperties`). | ||||||||||||||||||||||
|
||||||||||||||||||||||
| * When set, inaccessible (protected and private) members throughout type T are | |
| * ignored as if not present. Otherwise, inaccessible members are considered | |
| * an error (type checking will mention `SerializationErrorPerNonPublicProperties`). | |
| * When set, inaccessible (protected and private) members throughout type T are | |
| * ignored when computing derived types. | |
| * This has no impact on how the such fields are handled at runtime: | |
| * For this setting to match runtime behavior all public fields must be enumerable own properties, | |
| * and all inaccessible ones (if any) must be either inherited, symbol keyed or non-enumerable | |
| * When this option is not set, inaccessible members are considered | |
| * an type error (type checking will mention `SerializationErrorPerNonPublicProperties`). |
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 setting shouldn't have anything to do with public properties enumerable or not. But that comment is appropriate for JsonStringify assuming that is requirement of JSON.stringify.
jason-ha marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Copilot
AI
Sep 19, 2025
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.
[nitpick] The type assertion as is used to cast JSON.stringify to the branded return type. While this works, consider adding a comment explaining why this casting is safe and necessary for the branded type system.
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 see these two are just redefinitions of native functions but feels like they should still go to core-utils to keep this package types-only?
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.
Package cannot be type-only because it has enums.
core-utils does not depend on core-interfaces. If relocated, they could go to client-utils.
Since it can't be type-only and relocation would involve adding test exports to core-interfaces for all of the testing, I am inclined to say this is okay. But I will defer to the prevailing wisdom.
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.
Ah yes, client-utils is the one I was thinking of.
I can't think of strong reasons to oppose exporting them from here, but don't feel super confident being sole approver. I'd suggest getting more eyes on this. @anthony-murphy comes to mind as someone who might have comments.
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'm not really grasping the implications of this change, if there are concerns please bottom-out on those, despite my approval on the PR as a whole
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.
It is mostly a policy / positioning for this package to be type-only and not have any runtime contribution. But that is not the case with these packages anyhow because enums or enum-like types. I do not want us to start down a slippery slope. I am convinced the Json* type filters and JsonString belong here. I think it is okay to has JsonStringify and JsonParse colocated when there are just retypings of existing functions.