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.

Sign up for GitHub

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

Jump to bottom

[relay-compiler] Type-safe updaters for TypeScript #4370

Closed
wants to merge 34 commits into from
Closed

[relay-compiler] Type-safe updaters for TypeScript #4370

wants to merge 34 commits into from

Conversation

tobias-tengler
Copy link
Contributor

@tobias-tengler tobias-tengler commented Jul 9, 2023 •
edited
Loading

TypeScript supports specifying different getter and setter types since 5.1, so I've updated the type generation logic around @updatable and @assignable to work with TypeScript.

I've also updated the notice on the website and made the documentation around type-safe updaters available externally (if this is not wanted, just revert 33612d2).

Companion PR for @types/relay-runtime: DefinitelyTyped/DefinitelyTyped#66013 (Merged)

Closes #4212 #3965

@tobias-tengler tobias-tengler changed the title Update @updatable/@assignable type generation for TypeScript Update type-safe updater type generation for TypeScript Jul 9, 2023
@tobias-tengler tobias-tengler changed the title Update type-safe updater type generation for TypeScript Type-safe updaters for TypeScript Jul 9, 2023
captbaritone
captbaritone requested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@captbaritone captbaritone left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've left some comments, but overall I think this should be possible to land.

compiler/crates/relay-typegen/src/write.rs Outdated Show resolved Hide resolved
website/docs/guided-tour/updating-data/imperatively-modifying-linked-fields.md Outdated Show resolved Hide resolved
...gen/tests/generate_typescript/fixtures/updatable-fragment-spread-and-regular-spread.expected Show resolved Hide resolved
...gen/tests/generate_typescript/fixtures/updatable-fragment-spread-and-regular-spread.expected Show resolved Hide resolved
...relay-typegen/tests/generate_typescript/fixtures/updatable-fragment-spread-multiple.expected
export type updatableFragmentSpreadAndRegularSpread_2_updatable_user$key = {
readonly " $data"?: updatableFragmentSpreadAndRegularSpread_2_updatable_user$data;
readonly $updatableFragmentSpreads: FragmentRefs<"updatableFragmentSpreadAndRegularSpread_2_updatable_user">;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

In Flow this object, and updatableFragmentSpreadAndRegularSpread_2_updatable_user$data above is inexact (...). I don't have a good sense of why, but presumably that means we need some TypeScript equivalent? Or maybe it can be removed from the Flow types?

https://flow.org/en/docs/types/objects/#exact-and-inexact-object-types

Copy link
Contributor Author

@tobias-tengler tobias-tengler Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

How would such an inexact type be typed in Typescript, would it just be a union of

original_obj & { [k: string]: unknown };

Might need more input from some TypeScript wizard before I add this...

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can first start with understanding why its inexact in Flow? What breaks if we make it inexact? Is this specific to updatable fragments, or are all fragment keys inexact in Flow?

Maybe the semantics of TypeScript are different and we don't actually need them to be inexact in TypeScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other places inexact objects are used for fragments, if their MaskStatus is unmasked. I know too little about how fragments work to infer why it would be important to have the same characteristics when updating a fragment...

I tried tracing the git log for an explanation of why an InexactObject was being used, but haven't found anything. @rbalicki2 you seem to have added the code, do you perhaps remember why the InexactObject was being used?

.../relay-typegen/tests/generate_typescript/fixtures/updatable-fragment-spread-multiple.graphql Outdated Show resolved Hide resolved
...egen/tests/generate_typescript/fixtures/updatable-operation-plural-field-no-spreads.expected Show resolved Hide resolved
...en/tests/generate_typescript/fixtures/updatable-operation-plural-field-with-spreads.expected Outdated Show resolved Hide resolved
...n/tests/generate_typescript/fixtures/updatable-operation-assignable-fragment-plural.expected Outdated Show resolved Hide resolved
...-typegen/tests/generate_typescript/fixtures/updatable-operation-assignable-fragment.expected Outdated Show resolved Hide resolved
captbaritone
captbaritone reviewed Jul 17, 2023
View reviewed changes
compiler/Cargo.lock Outdated Show resolved Hide resolved
@tobias-tengler
Copy link
Contributor Author

@captbaritone I addressed everything that I could, please give it another look 🙏

@captbaritone
Copy link
Contributor

@tobias-tengler Thanks for the quick turn around. Left a few more comments.

Have you tried running this with a real TypeScript project? Does it generate valid types that appear to work with a minimal example of using typesafe updaters? Would love to have validation that this works end to end.

@tobias-tengler tobias-tengler force-pushed the typesafe-updaters-ts branch 2 times, most recently from a2b96ad to b07e96d Compare July 18, 2023 17:09
@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Jul 18, 2023 •
edited
Loading

@captbaritone I've built an example here.

While testing I noticed that the babel-plugin-relay doesn't behave correctly.
When having an @assignable fragment, the generated artifact will only export a validate function, not an importable node. babel-plugin-relay doesn't yet know about this, so it tries to replace the graphql tag with an import - which fails of course, since there's nothing to import.

I've fixed this in dbcc8bb. Do you think that is fine? Meta probably uses another transform, if this hasn't led to issues for you yet, right?

If we want to go forward like this, I can also submit a PR for Next.js to update their transform. vite-plugin-relay should be covered by the update to babel-plugin-relay.

Note: The example project is only runnable, since I moved the @assignable fragment to another file that isn't being referenced. I tried to import the locally built babel-plugin-relay, but failed miserably...

@tobias-tengler tobias-tengler force-pushed the typesafe-updaters-ts branch 5 times, most recently from aa5fafb to 9528ab8 Compare July 18, 2023 21:09
@tobias-tengler tobias-tengler changed the title Type-safe updaters for TypeScript [relay-compiler] Type-safe updaters for TypeScript Aug 8, 2023
@tobias-tengler tobias-tengler mentioned this pull request Aug 10, 2023
Closed
@tobias-tengler
Copy link
Contributor Author

@captbaritone sorry for the ping, I'm sure you're very busy, but I don't want this to stall. Could you take a look at my above comments or point out another member of the Relay Team that could do so?

@captbaritone
Copy link
Contributor

Thanks for the ping and sorry for the delay. I'll look in the morning!

captbaritone
captbaritone requested changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@captbaritone captbaritone left a comment

Choose a reason for hiding this comment

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

This is great! Can we split the addition of hashes into its own PR? After that I can import.

compiler/crates/relay-compiler/src/artifact_content/content.rs Outdated Show resolved Hide resolved
@tobias-tengler
Copy link
Contributor Author

@captbaritone I moved the addition of the node to the following PR: #4409

@tobias-tengler tobias-tengler mentioned this pull request Aug 18, 2023
Closed
@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Aug 27, 2023 •
edited
Loading

@captbaritone @alunyov bump 🙈

@facebook-github-bot
Copy link
Contributor

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@tobias-tengler tobias-tengler mentioned this pull request Sep 20, 2023
Open
@facebook-github-bot
Copy link
Contributor

@monicatang merged this pull request in 9c7b92a.

@michaelmcneilnet
Copy link
Contributor

Will this be released soon? This is a very valuable fix for us.

@tobias-tengler
Copy link
Contributor Author

tobias-tengler commented Oct 9, 2023 •
edited
Loading

I think the team is thinking about cutting a 15.1.0 release soon, see #4440
Should probably be a few weeks out. if you can't wait you can always use the relay-compiler@main tag.

@tobias-tengler tobias-tengler deleted the typesafe-updaters-ts branch October 9, 2023 16:47
@tobias-tengler tobias-tengler mentioned this pull request Nov 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captbaritone captbaritone captbaritone requested changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New tutorial uses mutation APIs not available for TypeScript users
4 participants
@tobias-tengler @captbaritone @facebook-github-bot @michaelmcneilnet