More specific JSON parse reviver argument in lib.d.ts #6955

Activity

1. mhegazy commented on Feb 7, 2016

   

   Mohamed Hegazy (mhegazy)

   on Feb 7, 2016

   Contributor

   More actions

   The problem here is similar to for..in issues, see the fix in TS 1.8 in #6379.

   The value is a string, but it could be a numeric string in cases of array. it is not a number value (i.e. can not call toExponent on it) but it can still be used to index into an array (since at the end all index operations first do a toString in the key anyways). TypeScript, tries to strongly type index operations using index signatures. there are two, string and numeric. and if you index into an object using a type that it does not have an index signature for, you get an any. ideally, this

   so:

   var list = [0, 1];
   JSON.parse("[3, 4]", (k:string, v) => {
       var e = list[k]; // e is any, as list has no string indexer,
                             // also an error with --noImplicitAny
   })
   
   JSON.parse("[3, 4]", (k:number, v) => {
       var e = list[k]; // e number, as expected
       k.toExponential() // runtime error, k is not a number
   })

   in short, this was intentional. but maybe we should reconsider.
2. Mohamed Hegazy (mhegazy)

   added

   SuggestionAn idea for TypeScriptAn idea for TypeScript

   In DiscussionNot yet reached consensusNot yet reached consensus

   on Feb 7, 2016
3. avonwyss commented on Feb 7, 2016

   

   Arsène von Wyss (avonwyss)

   on Feb 7, 2016

   Author

   More actions

   I understand your reasoning, but how is this different from the stringify replacer which is kind of a sibling to the reviver?

   stringify(value: any, replacer: (key: string, value: any) => any): string;
   

   I see this as a callback with clearly defined arguments, the first being a string. Nothing prevents you from passing in a function which accepts (any, any), but it would prevent you from passing in (number, any) which is currently allowed but wrong to my understanding:

   function B(name: number, value: any): any {
       ...
   } 
   
   JSON.parse("", B); // no error as of now!
   
4. Ryan Cavanaugh (RyanCavanaugh)

   added

   Help WantedYou can do thisYou can do this

   Good First IssueWell scoped, documented and has the green lightWell scoped, documented and has the green light

   and removed

   In DiscussionNot yet reached consensusNot yet reached consensus

   on May 9, 2016
5. RyanCavanaugh commented on May 9, 2016

   

   Ryan Cavanaugh (RyanCavanaugh)

   on May 9, 2016

   Last edited by RyanCavanaugh

   Member

   More actions

   PR welcomed on this one (be sure to read CONTRIBUTING.md on how to submit a lib.d.ts PR)
6. Mohamed Hegazy (mhegazy)

   added this to the CommunityThis milestone has been deleted milestone on Jan 4, 2018
7. AnyhowStep commented on Oct 31, 2018

   

   AnyhowStep

   on Oct 31, 2018

   Last edited by AnyhowStep

   Contributor

   More actions

   Hey,

   I've been wanting to get more involved with the TypeScript code base but simply lack the knowledge about the current implementation (code base is huge and intimidating, I've only written toy interpreters).

   This issue looks simple enough, I think. Just seems to be updating .d.ts files?

   ---

   I looked at DefinitelyTyped/DefinitelyTyped#15744 and their Reviver type,

   export type Reviver = (this: {}, key: string, value: any) => any;

   I think the this parameter should be of type any or the following code will fail in strict mode,

   JSON.parse(`{ "x":42 }`, function (k, v) {
       //Element implicitly has an 'any' type because type '{}' has no index signature.
       const x = this[k];
   });

   If we use this : any, then x will be of type any in the above code.

   ---

   It also looks like JSON.stringify() needs to be updated.
   The replacer parameter looks like it could benefit from having the same signature as Reviver

   ---

   • Should JSON.parse()'s reviver be (this: any, key: string, value: any) => any?
   • Should JSON.stringify()'s replacer be (this: any, key: string, value: any) => any?

   ---

   I ran grep -nri reviver\? ./ and grep -nri replacer\? ./ in the lib/ directory and only found results in lib.es5.d.ts, I assume that's the only file that needs to be updated?
8. AnyhowStep commented on Oct 31, 2018

   

   AnyhowStep

   on Oct 31, 2018

   Contributor

   More actions

   

   I also just noticed there are trailing spaces in 4 lines of comments in lib.es5.d.ts.

   I assume no one wants these removed.

   ---

   I made the changes in the two places and git diff gives me this,

   

   ---

   Assuming this is all good, do I just need to run jake tests, and jake runtests before submitting a PR?

   ---

   Also, I tried signing the CLA (Contributor License Agreement) at https://cla.opensource.microsoft.com/ but I think it's broken.
   I signed in to GitHub and authorized the site to access my public account info and it's still asking me to sign in. Clicking "Sign In" again doesn't ask me to authorize the site again; so I assume authorization has been given but something else is broken.

   Unless signing in was the same as signing the CLA...
9. AnyhowStep

   added a commit that references this issue on Oct 31, 2018

   Fixes microsoft#6955

   This issue will close once commit 01e7182 is merged into the 'master' branch.

   01e7182
10. Ryan Cavanaugh (RyanCavanaugh)

    closed this as completedin #28270on Feb 2, 2019
11. Ryan Cavanaugh (RyanCavanaugh)

    added a commit that references this issue on Feb 2, 2019

    Merge pull request #28270 from AnyhowStep/json-parse-reviver-stringif…

    ...

    0a041ee