TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse

Open clarfonthey opened this issue 2 years ago • 2 comments

Fixes #18879.

This reflects the types of the actual JSON.stringify function, which may return string | undefined. This approach chooses to add extra overloads specific to the JSON-valid types (specifically boolean | number | string | object) to allow the caller to ensure that the value is definitely a string. To make these overloads effective, the input that may potentially return undefined is marked with unknown, not any.

This also modifies the replacer to JSON.parse to follow the same format as the new replace for stringify, which now accepts unknown instead of any. To minimise the cascading of changes made, however, parse still returns any instead of unknown.

clarfonthey avatar Aug 09 '22 23:08 clarfonthey

CLA assistant check
All CLA requirements met.

ghost avatar Aug 09 '22 23:08 ghost

I'm going to try one more time, but I genuinely can't reproduce the errors happening on CI, locally. I rebased on main multiple times and ran multiple variations of the test commands, including literally copying the same stuff CI does, and it just did not result in the same errors locally. Everything passes on my end. node --version is 18.7.0 and it doesn't matter whether I do a regular npm install or npm ci; it just doesn't work.

clarfonthey avatar Aug 10 '22 23:08 clarfonthey

Note to any TypeScript maintainers reading this: the fact that baselines hard-code the line and column of items in the library source code makes this very difficult to properly maintain. Given how changes seem to be merged daily that change these files, this makes library improvements additionally painful, even if it's for a very important change.

I don't blame the actual team members responsible for reviewing PRs, but would like to specifically call out Microsoft, a several-billion-dollar company, for not allocating the resources to the TS team to adequately review PRs to what has become a critical piece of infrastructure for many companies. This is not a volunteer project, but something being developed by a for-profit corporation with control of much of the Node ecosystem.

I'm willing to continue to put up with merging in the main branch for another week or so, but beyond that, I'm going to just give up and face the fact that this isn't going to be merged. I'm almost certain this is the reason why this bug, which has had multiple fix PRs opened since it was initially reported, has never been fixed-- the project changes rapidly, is fragile to these sorts of changes, and there's inadequate staff allocated to actually helping contributors merge changes in a manner that reflects that.

Doubt anyone's actually going to be able to take action on this, but figured I'd post a comment for those burned by this bug to read and sympathise with. Hopefully, this doesn't matter too much and this actually does get merged.

clarfonthey avatar Aug 17 '22 06:08 clarfonthey

The CI runs a second pass to ensure that TypeScript can still build itself after any proposed change. You introduced build errors into shims.ts, as shown on the PR page itself:

image

You can reproduce this locally by running

gulp lkg
gulp local

I haven't seen a PR break the build in this particular way before, but we should probably update the CONTRIBUTING.md instructions to address this edge case.

RyanCavanaugh avatar Aug 17 '22 21:08 RyanCavanaugh

So I should add, this particular case came up after the latest change, but before I was getting other baseline diffs showing up that I could not actually get to generate locally, no matter how hard I tried; it just said that the files were generated. This change is actually very clear in the diffs that run in CI.

I'll see if I can add these steps to CONTRIBUTING.md. Thank you for helping out. ❤️

For the specific build I'm referencing: https://github.com/microsoft/TypeScript/runs/7757158610

clarfonthey avatar Aug 17 '22 21:08 clarfonthey

So I don't understand why CI is passing now -- I didn't finish fixing all the errors last night and they're still showing up for me locally, so, I'm planning to do more work later on it.

clarfonthey avatar Aug 18 '22 17:08 clarfonthey

I've been mostly unable to work on this this week due to real-life moving hell, but was planning to continue this either next week or the week after.

In terms of the object literal extraction, to my understanding, it's because the type checking literally differs between lvalues and rvalues, or literals and bindings, or whatever pair of terms you want to use. It prefers to treat the value as unknown rather than directly inferring object.

I chose specifically object instead of some other type like a Record since tsc infers this directly when checking typeof, and it's the type that most closely matches the spec. But it's unhappy using this type when directly comparing object literals for some unknown reason. If you can figure out the cause of this, I'm fine undoing my changes.

I was testing via npx gulp lkg && npx gulp tests; I didn't want to run in parallel since I wanted to do other things while it was running.

clarfonthey avatar Sep 02 '22 12:09 clarfonthey

Yeah I, lost all desire to work on this, so, I'm just going to close this PR so people don't think I'm going to try this again.

clarfonthey avatar Dec 03 '22 03:12 clarfonthey