feat(common): support custom JSON parser in HttpClient by trotyl · Pull Request #25027 · angular/angular

trotyl · Jul 22, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #21079

When user works with an extended JSON format, eg. JSON5, due to it's not compatible with JSON.parse(), user have to make it raw text and parse it manually.

But JSON5 format is still conceptually 'json' rather than 'text', it's a fully superset of ES JSON, just being more human friendly.

What is the new behavior?

Simply with:

@NgModule({
  providers: [
    { provider: JsonParser, useValue: JSON5 }
  ]
})
class AppModule {}

Or strip a different receiver:

class MyJsonParser implements JsonParser {
  parse(text: string, reviver?: (key: any, value: any) => any): any {
    const strippedText = text.trimStart(CUSTOM_XSSI)
    return JSON.parse(strippedText, reviver)
  }
}

Or have a different reviver:

class MyJsonParser implements JsonParser {
  parse(text: string, reviver?: (key: any, value: any) => any): any {
    const wrappedReviver = wrapReviver(reviver)
    return JSON.parse(text, wrappedReviver)
  }
}

Then server could provide JSON5 response directly and no error would be raised.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

packages/common/http/src/module.ts

packages/common/http/src/xhr.ts

Lonli-Lokli · Sep 4, 2019

What should be done to accept this PR?

closes angular#21079

petebacondarwin

@trotyl thanks for this PR. I am sorry it slipped under our radar for so long. I rebased it and cleaned up the conflicts. But I have a few suggestions for how to make it a bit slimmer.

petebacondarwin · Jan 26, 2021

packages/common/http/src/xhr.ts

 @Injectable()
 export class HttpXhrBackend implements HttpBackend {
-  constructor(private xhrFactory: XhrFactory) {}
+  constructor(private xhrFactory: XhrFactory, @Inject(JSON_PARSER) private jsonParser: JsonParser) {


How about making this optional, then we wouldn't have to define a provider for the default case:

Suggested change

constructor(private xhrFactory: XhrFactory, @Inject(JSON_PARSER) private jsonParser: JsonParser) {

constructor(private xhrFactory: XhrFactory, @Optional() @Inject(JSON_PARSER) private jsonParser?: JsonParser) {

if (jsonParser == null) { // note == matches both `null` and `undefined`

this.jsonParser = JSON;

}

petebacondarwin · Jan 26, 2021

packages/common/http/test/xhr_spec.ts

    beforeEach(() => {
      factory = new MockXhrFactory();
-      backend = new HttpXhrBackend(factory);
+      backend = new HttpXhrBackend(factory, JSON);


If jsonParser was optional this would not need to change.

petebacondarwin · Jan 26, 2021

packages/common/http/src/xhr.ts

 const XSSI_PREFIX = /^\)\]\}',?\n/;

+/**
+ * Utility for parsing JSON text to JavaScript object


I think we would need a bit more documentation for this and the JSON_PARSER token below, ideally with an example and referencing each other.

petebacondarwin · Jan 26, 2021

packages/common/http/src/module.ts

+export function _JSON(): JsonParser {
+  return JSON;
+}


If jsonParser was optional then this would not be needed.

petebacondarwin · Jan 29, 2021

We discussed this in the team meeting yesterday. The general consensus is that this is actually fairly straightforward to do via an interceptor and that the real problem is the lack of discoverability of this approach to solving the problem.

We accept that the problem is compounded by the fact that HttpClient defaults to json parsing with its own parser and so the natural solution that people lean towards is replacing this built-in JSON parsing directly. It would have been better if the default had been text so that JSON parsing felt more like a special case that would lead more naturally to overriding via an interceptor.

The proposal then, is not to merge this feature into core, but instead to improve the documentation to lead developers clearly to an interceptor based solution.

Here is a link to an example of how custom JSON parseing could be done in a reusable interceptor: https://stackblitz.com/edit/angular-ivy-nrdj5q?file=src%2Fapp%2Fapp.module.ts. Note that this could be wrapped up and distributed as a 3rd party library and then the application developer would only need to provide their own JsonParser class.

angular-automatic-lock-bot · Mar 1, 2021

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Jul 22, 2018

trotyl force-pushed the json-custom-parse branch 2 times, most recently from 6ed46ea to 18ad2a9 Compare July 22, 2018 13:51

JoostK reviewed Jul 22, 2018

View reviewed changes

packages/common/http/src/module.ts Outdated Show resolved Hide resolved

alfaproject reviewed Jul 22, 2018

View reviewed changes

packages/common/http/src/xhr.ts Outdated Show resolved Hide resolved

trotyl force-pushed the json-custom-parse branch from 18ad2a9 to 31a9d98 Compare July 23, 2018 05:00

vicb added area: common Issues related to APIs in the @angular/common package area: common/http Issues related to HTTP and HTTP Client and removed area: common/http Issues related to HTTP and HTTP Client labels Jul 23, 2018

pkozlowski-opensource added area: common/http Issues related to HTTP and HTTP Client and removed area: common Issues related to APIs in the @angular/common package labels Dec 19, 2018

ngbot bot added this to the needsTriage milestone Dec 19, 2018

feat(common): support custom JSON parser in HttpClient

65a7751

closes angular#21079

petebacondarwin force-pushed the json-custom-parse branch from 31a9d98 to 65a7751 Compare January 26, 2021 19:30

pullapprove bot requested a review from alxhub January 26, 2021 19:30

petebacondarwin suggested changes Jan 26, 2021

View reviewed changes

petebacondarwin mentioned this pull request Jan 28, 2021

Ability to configure custom JSON.parse reviver for the HttpClient #21079

Closed

petebacondarwin closed this Jan 29, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): support custom JSON parser in HttpClient#25027

feat(common): support custom JSON parser in HttpClient#25027
trotyl wants to merge 1 commit intoangular:masterangular/angular:masterfrom
trotyl:json-custom-parsetrotyl/angular:json-custom-parseCopy head branch name to clipboard

trotyl commented Jul 22, 2018 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Lonli-Lokli commented Sep 4, 2019

Uh oh!

petebacondarwin left a comment

Uh oh!

petebacondarwin Jan 26, 2021

Uh oh!

petebacondarwin Jan 26, 2021

Uh oh!

petebacondarwin Jan 26, 2021

Uh oh!

petebacondarwin Jan 26, 2021

Uh oh!

petebacondarwin commented Jan 29, 2021

Uh oh!

angular-automatic-lock-bot bot commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

-  constructor(private xhrFactory: XhrFactory, @Inject(JSON_PARSER) private jsonParser: JsonParser) {
+  constructor(private xhrFactory: XhrFactory, @Optional() @Inject(JSON_PARSER) private jsonParser?: JsonParser) {
+  if (jsonParser == null) { // note == matches both `null` and `undefined`
+    this.jsonParser = JSON;
+  }

Search code, repositories, users, issues, pull requests...

Conversation

trotyl commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Uh oh!

Uh oh!

Lonli-Lokli commented Sep 4, 2019

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Jan 29, 2021

Uh oh!

angular-automatic-lock-bot bot commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

trotyl commented Jul 22, 2018 •

edited

Loading