-
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 all commits
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 |
|---|---|---|
|
|
@@ -7,9 +7,8 @@ | |
| // eslint-disable-next-line no-restricted-syntax, @typescript-eslint/no-restricted-imports | ||
| export * from "./index.js"; | ||
|
|
||
| // Important: all other exports must be type only exports. In package.json exports, | ||
| // index.js is listed as the runtime file. This is done so that all imports are | ||
| // using the same outer runtime file. (Could be changed if needed.) | ||
| export type { JsonString, JsonStringifyOptions } from "./jsonString.js"; | ||
| export { JsonStringify, JsonParse } from "./jsonString.js"; | ||
|
Contributor
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 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?
Contributor
Author
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. Package cannot be type-only because it has enums.
Contributor
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. 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.
Member
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'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
Contributor
Author
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. 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. |
||
|
|
||
| export type { JsonTypeToOpaqueJson, OpaqueJsonToJsonType } from "./jsonUtils.js"; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| /*! | ||
| * 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 { InternalUtilityTypes } from "./exposedInternalUtilityTypes.js"; | ||
| import type { JsonDeserialized } from "./jsonDeserialized.js"; | ||
| import type { JsonSerializable, JsonSerializableOptions } 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(); | ||
| } | ||
|
|
||
| /** | ||
| * Distributes `JsonStringBrand` over union elements of T. | ||
| * | ||
| * @remarks | ||
| * This is useful to allow `JsonString<A | B>` to be assigned to `JsonString<A> | JsonString<B>`. | ||
| * | ||
| * The downside is that enums are expanded to union of members and thus cannot be | ||
| * reconstituted exactly (even if IntelliSense shows the original enum type). This | ||
| * can be removed if exact enum preservation is found to be more important. | ||
|
Member
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 think we like avoiding enums anyway so seems like the right priority
Contributor
Author
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. So that helper has that problem. Then we use the other helper - lines below to manage that. |
||
| */ | ||
| type DistributeJsonStringBrand<T> = T extends unknown ? JsonStringBrand<T> : never; | ||
|
|
||
| /** | ||
| * Distributes branding over union elements of T unless result could prevent T | ||
| * from being reconstituted (as in the case of an enum type), in which case it | ||
| * falls back to a single JsonStringBrand for the entire T. | ||
| * | ||
| * @remarks | ||
| * Note that an enum unioned with anything else will be distributed. It seems | ||
| * however that TypeScript can/will reconstitute the enum type in that case. | ||
| */ | ||
| type BrandForJsonString<T> = InternalUtilityTypes.IfSameType< | ||
| DistributeJsonStringBrand<T> extends JsonStringBrand<infer U> ? U : never, | ||
| T, | ||
| DistributeJsonStringBrand<T>, | ||
| JsonStringBrand<T> | ||
| >; | ||
|
|
||
| /** | ||
| * 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 & BrandForJsonString<T>; | ||
|
|
||
| /** | ||
| * Compile options for {@link JsonStringify}. | ||
| * | ||
| * @remarks | ||
| * This only impacts type checking -- it has no impact on runtime. | ||
| * | ||
| * The options are currently a subset of {@link JsonSerializableOptions}, specifically | ||
| * only `IgnoreInaccessibleMembers` is supported. | ||
| * | ||
| * No instance of this should ever exist at runtime. | ||
| * | ||
| * @privateRemarks | ||
| * Consider adding `AllowUnknown` option to allow precisely `unknown` types to | ||
| * be passed through. With `unknown` expected successful serialization could not | ||
| * be checked at compile time. At deserialization time, `unknown` does not | ||
| * guarantee any type and thus allowing does not erode type safety. | ||
| * | ||
| * @internal | ||
| */ | ||
| export type JsonStringifyOptions = Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">; | ||
|
|
||
| /** | ||
| * Performs basic JSON serialization using `JSON.stringify` and brands the result as {@link JsonString}`<T>`. | ||
| * | ||
| * @remarks | ||
| * Parameter `value` must be JSON-serializable and thus type T is put through filter {@link JsonSerializable}. | ||
| * | ||
| * @internal | ||
| */ | ||
| export const JsonStringify = JSON.stringify as < | ||
| T, | ||
| Options extends JsonStringifyOptions = Record<never, never>, | ||
| >( | ||
| value: JsonSerializable< | ||
| T, | ||
| // Make sure only options that are known are passed through. | ||
| Pick<Options, Extract<keyof JsonStringifyOptions, keyof Options>> | ||
| >, | ||
| ) => JsonString<T>; | ||
|
|
||
| /** | ||
| * Performs basic JSON parsing using `JSON.parse` given a {@link JsonString}`<T>` (`string`). | ||
| * | ||
| * @remarks | ||
| * Return type is filtered through {@link JsonDeserialized}`<T>` for best accuracy. | ||
| * | ||
| * Note that `JsonParse` cannot verify at runtime that the input is valid JSON | ||
| * or that it matches type T. It is the caller's responsibility to ensure that | ||
| * the input is valid JSON and the output conforms to the expected type. | ||
| * | ||
| * @internal | ||
| */ | ||
| export const JsonParse = JSON.parse as <T extends JsonString<unknown>>( | ||
| text: T, | ||
| ) => T extends JsonString<infer U> ? JsonDeserialized<U> : unknown; | ||
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.
Its unclear how somone is supposed to find the docs on JsonSerializableOptions: IgnoreInaccessibleMembers from here.
Up on line 871 I think you should replace
with
Pick<JsonSerializableOptions, "IgnoreInaccessibleMembers">so its clear those are the same field, and that documentation applies.
THen here you could do:
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.
That would be a cyclic file import unless that type is relocated, which didn't feel the best for readability for the API file. I am trying to keep the exposed options a bit separated from the internals. See also internal
FilterControlsthat is like part of theJsonSerializableOptions-- I don't want to have those interdependent.But, yes, there are no docs now in this place for that internal option. I will add a comment.