facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.4k stars 1.82k forks source link

Providers as modules or relative paths #4456

Open ianobermiller opened 1 year ago

ianobermiller commented 1 year ago

We're adopting Relay for the Netflix TVUI application, and one pain point we've come across is that we have providers that we want to share across our app, but because provider is always interpreted as a file relative to the fragment (as of https://github.com/facebook/relay/commit/9393fe5b75d95df37daaab750f13ea72b4420be4) we can't do that.

This would either be a breaking change or have to be behind a flag.

Right now the following fragment

fragment useBobHeaderQuery_titleImageFragment on Video
    @argumentDefinitions(
        titleImageSize: { type: "ArtworkDimension", provider: "./useBobHeaderQuery_titleImage.relayprovider" }
        artworkCapability: { type: "ArtworkCapability", provider: "@tvui/providers/artworkCapability" }
) {
    ...
}

Compiles to:

  "__relay_internal__pv__useBobHeaderQuery_titleImagerelayprovider": require('./../useBobHeaderQuery_titleImage.relayprovider'),
  "__relay_internal__pv__tvuiprovidersartworkCapability": require('./@tvui/providers/artworkCapability')

Our proposal is that it will instead resolve to:

  "__relay_internal__pv__useBobHeaderQuery_titleImagerelayprovider": require('./../useBobHeaderQuery_titleImage.relayprovider'),
  "__relay_internal__pv__tvuiprovidersartworkCapability": require('@tvui/providers/artworkCapability')

In other words, if the path starts with a . it is interpreted as relative, and if not, it is global. This also brings these module strings more in-line with how CommonJS resolvers work.

Ideally this change would apply to all such module strings, not just providers. This would also have a side benefit of removing some of the custom code around Haste module handling for JS modules.

Finally just want to note that I am happy to contribute this feature if you'd be willing to accept it!

cc @alunyov, @Lalitha-Iyer

Lalitha-Iyer commented 1 year ago

cc @captbaritone

alunyov commented 1 year ago

In other words, if the path starts with a . it is interpreted as relative, and if not, it is global. This also brings these module strings more in-line with how CommonJS resolvers work.

I think this make sense, also I agree that we should use CommonJS approach:

If the module identifier passed to require() [...] does not begin with '/', '../', or './'

From: https://nodejs.org/api/modules.html#loading-from-node_modules-folders

And otherwise, we may assume it is a global/haste module identifier.

Finally just want to note that I am happy to contribute this feature if you'd be willing to accept it!

Thank you, yes!