Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add UTF-8 APIs on TextEncoder and add JavascriptEncoder to JsonEncodedText #39415

Merged
merged 11 commits into from
Jul 15, 2019

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jul 11, 2019

Replacement for PR #38947 which was started by @ahsonkhan

This is the first phase of https://github.com/dotnet/corefx/issues/37192; additional phases will allow the custom encoders to be specified in writer and serializer options.

  • Update tests to have them succeed (match semantics of JavaScriptEncoder) and add new tests.
  • Change encoding of forward slash from "u002f" to "/" to match current semantics of JavaScriptEncoder. Note this will likely change soon to not escape at all per @GrabYourPitchforks.
  • Improved perf of ascii scenarios in JavaScriptEncoder from being 4x slower to ~25% slower (1.25x) than the implementation in System.Text.Json.
  • In order to prevent a regression of ~25% by default for ascii (when not specifying a custom encoder) the current code in System.Text.Json was kept. In addition, the code was modified to improve custom encoding scenarios by ~15%. If a non-ascii (latin) chacter >= 128 is used, then the default JavaScriptEncoder is used.

@steveharter steveharter self-assigned this Jul 11, 2019
@steveharter steveharter added this to the 3.0 milestone Jul 12, 2019
@steveharter
Copy link
Member Author

/azp run corefx-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveharter steveharter changed the title Javascript encoder Add UTF-8 APIs on TextEncoder and add JavascriptEncoder to JsonEncodedText Jul 12, 2019
@ericstj
Copy link
Member

ericstj commented Jul 12, 2019

Build failures are occurring because system.text.json is inbox on uap but text.encoding.web is not.

<IsNETCoreApp>true</IsNETCoreApp>

To fix this, make s.t.e.w inbox on uap. See example 10fc6ee

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to DecodeScalarValueFromUtf8 is the only thing that unsettled me, everything else is primarily just a nit.

@@ -55,12 +55,12 @@ public static void WritePrimitives()

{
Uri uri = new Uri("https://domain/path");
Assert.Equal(@"""https:\u002f\u002fdomain\u002fpath""", JsonSerializer.Serialize(uri));
Assert.Equal(@"""https:\/\/domain\/path""", JsonSerializer.Serialize(uri));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the offline conversation, there was a desire to change '/' to go through unescaped by default. Feel free to make this change if you wish, otherwise it will eventually be addressed per https://github.com/dotnet/corefx/issues/39445.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be good to handle that in a separate PR.

@steveharter steveharter merged commit bbbcac5 into dotnet:master Jul 15, 2019
@steveharter steveharter deleted the JavascriptEncoder branch July 15, 2019 16:46
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants