Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsonify: add jump to definition and any support #519

Merged
merged 21 commits into from Nov 28, 2022

Conversation

sachinraja
Copy link
Contributor

@sachinraja sachinraja commented Nov 26, 2022

These changes are upstreamed from work I did for tRPC and Remix.

For data inferred from Jsonify, properties do not have jump to definition. This is because key remapping which breaks jump to definition support for TS (see microsoft/TypeScript#47813). I changed it to use a different strategy for filtering that keeps jump to definition.

I also added support for the any type, previously Jsonify<any> was never and other variations of that were incorrect too.

I refactored the implementation because the previous one did not flow well with these changes.

cc @MichaelDeBoey

@sachinraja sachinraja marked this pull request as draft November 26, 2022 07:54
@sachinraja sachinraja marked this pull request as ready for review November 26, 2022 15:20
@sachinraja sachinraja changed the title jsonify: add jump to definition support jsonify: add jump to definition and any support Nov 26, 2022
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Thanks @sachinraja, this is really awesome! 🤩

Once this one's merged, we can start using it in the Remix codebase 😍

source/jsonify.d.ts Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

Forgot to mention these 🙈

No problem if you decide to not change these though 🤷‍♂️

source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
source/jsonify.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 2071f47 into sindresorhus:main Nov 28, 2022
@sindresorhus
Copy link
Owner

Nice work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants