TypeScript
TypeScript copied to clipboard
Fix JSON.stringify return type (#18879), reduce usage of any in JSON.parse
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
.
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.
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.
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](https://user-images.githubusercontent.com/6685088/185243450-444cb9c7-c206-4d94-b335-67d6cf29d9cc.png)
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.
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
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.
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.
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.