Fix Relay Resolver typegen for Typescript-based projects

[ernieturner]

It seems like Relay 15 has a bug where when generating Typescript code for Relay Resolvers, the import of the Relay Resolver file is no longer included. This causes invalid TS generation since the codegen contains something like

ReturnType<typeof userPopStarNameResolverType>

But is missing the import from the Relay Resolver for the userPopStarNameResolverType type.

This PR removes some explicit exclusion of Typescript imports for Relay Resolvers that seems to have been added in error. I believe the idea was that the relay-codegen create would do the import of the Relay Resolver function, but that doesn't appear to be happening either.

This PR also adds some extra tests as well, however, I was hoping to find some suite of tests that showed the resulting output of both the codegen and typegen together, but I wasn't able to find that. If a suite of tests like that exists (or is simple to write) I'd definitely be up for adding it.

[Blitz2145]

I can confirm this bug with the TS codegen in relay 15. Not only is the type not imported, but I think there is another potential bug. The generated code does not seem to support resolver modules with a default export anymore, we had to switch to using named exports for the resolvers to get things working.

[davidmccabe]

@captbaritone Just flagging this for you

[facebook-github-bot]

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

[captbaritone]

Thank @ernieturner. I'll import this and see if I can merge it with a fix.

@Blitz2145 Can you try enabling eagerEsModules in your relay compiler config?

[ernieturner]

Hey @captbaritone did you ever figure out what might be wrong here?

[captbaritone]

It looks like this is a result of https://github.com/facebook/relay/blame/main/compiler/crates/relay-typegen/src/write.rs#L552 added in 0ef7043. The expectation is that in TypeScript, which does not have a special import type ... syntax, the module would already be imported by the codegen code, and typegen would just piggy back on that.

In other words, we expect actual runtime artifacts that are emitting the type of this resolver to also import the concrete resolver implementation/module which will get reused by this reference in the types.

Here the test only shows what typegen constructs, but not what we end up with when typegen and codegen get combined.

So the next thing to check would be to find a real world example where this is happening (outside of a test) and see why that name is not being imported by codegen. Is it somehow possible to construct a runtime artifact that includes the type of the field but not the actual code to read it? Is there some case where it gets imported under a different name?

[ernieturner]

Typescript does have an import type option, at least as of TS 3.8. I don't know if there's a minimum TS target we're shooting for with codegen though.

[tomgasson]

Just noting that Relay already has some import type support via useImportTypeSyntax: true. This is very meaningful because it allows you to avoid hard-requiring react-relay / relay-runtime when using artifacts in early-load.

I've noticed that there is some inconsistency with what's imported, with quite a few unused imports

import type { ConcreteRequest, Query } from 'relay-runtime';

(where we see Query is often unused) and imports being output in a mix of top-level statements and later inline calls, with it differing based on which feature is involved (3D, providers, resolvers, preloadable)

It's a little difficult to contribute in this area currently because of the slight mix between mechanisms used (relay-typegen, write_import_type, write_js_dependency, TopLevelStatement etc) and many of them currently occurring during printing rather than codegen.

I think it might be a good idea to switch all importing to be buffered (TopLevelStatement style) and context aware (importKind/source/local name/imported name) during codegen & printing and then output that all at once as a prefix to the printed content. Is that something you're open to contributions of?

[ernieturner]

@captbaritone I got a chance to dig into this a bit today, and I'm maybe now more confused about why this change was made to omit inclusion of the Relay Resolver module for Typescript files when that same function still performs that action for Flow modules. Is the assumption that for Flow we write the code in the typegen crate because the resulting import userPopStarNameResolver from "PopStarNameResolver"; style line in Flow is only importing a type?

I think the reason that the relay-codegen crate doesn't import the Relay Resolver is that it doesn't need to import anything for non-type purposes. For Typescript, the Relay Resolver function that gets imported is only ever used for the type construct of ReturnType<typeof {resolverFunction}>. I wasn't able to find any existing code in the relay-codegen crate that deals with Relay Resolvers at all.

Would you be ok if I put up a PR that removes the code I linked to above? Maybe the "correct" solution here is to add some amount of code to the relay-codegen crate to do this, but in the meantime it'd probably be good to get this fixed so that people (like me :)) can upgrade to Relay 15 in our project.

[captbaritone]

Looked at this a bit more today and yesterday. I believe the code you linked above (where we don't include the import for TypeScript code) should just get removed. TypeScript and Flow both need that import. Want to open a PR?

I think the confusion is that both codegen and typegen need the resolver imported. However, codegen and typegen are separate steps and don't know about eachother, and yet need to avoid namespace collisions.

So, we have two top level variable names where typegen appends Type to the end. But, despite the name, it's still importing the concrete function. The suffix merely avoids double declaring a top level variable name. So, while it's true that codegen has already imported the module (in both TS and Flow!) we can't use that name safely so it's better to just double import it under two names. I don't think that's invalid, and its what we do in Flow.

Longer term, we should develop a generic JavaScript AST that's used for both codegen and typegen. This would allow us to merge the two safely to avoid namespace collisions wile avoiding duplicate imports.

It would also allow us to hoist all imports to the top, which would look a lot cleaner.

[ernieturner]

@captbaritone Ok, updated the PR title/description/code to remove that TS check in the typegen. I seemed to recall that when we were talking last week you mentioned that there was a suite of tests that exercised the combination of both the typegen and the codegen, but I wasn't able to find where that might be. Based on the fact that I only had to touch unit test gen files in the relay-typegen crate I suspect that maybe that doesn't exist? Otherwise I'd expect some other CI failures to crop up.

Also note, I made some other minor file changes to get CI to a slightly better state with CI around yarn updates and lint fixes so there's a little bit of extra noise in the PR from that.

[facebook-github-bot]

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

[facebook-github-bot]

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

[facebook-github-bot]

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

[captbaritone]

Hey @ernieturner, looks like there's a merge conflict which is preventing us from re-importing this. Could you rebase?

[ernieturner]

@captbaritone Should be rebased against the latest of what's on main!

[facebook-github-bot]

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

[facebook-github-bot]

@captbaritone merged this pull request in 556d696.

[alloy]

@captbaritone @ernieturner This change is breaking the eagerESModules setting when disabled and using typescript, as it will now always emit an import. This could be considered as designed, we're only disabling that atm to workaround some other issue we have, however it does lead to unexpected results. Thoughts on the correct next step?

• Should using typescript and eager ES modules be mutually exclusive and a helpful error should be logged?
• Or should this be fixed to not emit when eager ES modules is disabled?

As an aside, I'm a bit confused about why it's ok to have the 2 imports for the same named export. The typegen variant (*Type) isn't being used in the artefacts I'm seeing produced for our live resolver. What gives?