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

[v14] Provided variables docs are wrong, or doesn't support esmodules #3966

Open bigfootjon opened 2 years ago

bigfootjon commented 2 years ago

I believe this is due to the Haste system used at Meta that handles JS modules differently, but the problem is this:

Following the docs I created a UserIDArgumentProvider.relayprovider.js file and used it in a fragment like so:

@argumentDefinitions(
  id: {
    type: "ID!"
    provider: "../user/UserIDArgumentProvider.relayprovider"
  }
)

(note the leading "../" on the provider path. It seems the argument provider import is relative to the output directory, which for me is <ROOT>/__generated__, so I needed to get back to <ROOT> but this is a tangent and might be worth a separate issue).

I then defined user/UserIDProvider.relayprovider.js as the docs indicate:

export default {
    get(): string {
        return "12345";
    },
};

This compiles just fine, but the problem is at runtime, where this line of code fails:

https://github.com/facebook/relay/blob/17636d63796d6daf8ae9059d5a37f5609067f198/packages/relay-runtime/store/RelayConcreteVariables.js#L102

For my configuration, it should be:

-operationVariables[varName] = providedVariables[varName].get();
+operationVariables[varName] = providedVariables[varName].default.get();

However, I assume that won't work at Meta, so I guess it should support both. I remember this coming up with some other Relay feature and can't remember the resolution to that problem, so I leave that to the maintainers.

The alternative solution is to update the OSS docs to indicate that this is the correct way to write the provider:

export default function get(): string {
    return "12345";
};

This version worked just fine for me.

I'm happy to put up a PR allowing a default or fixing the docs, but I'm not sure which is the right solution. Please let me know which way is preferred.

captbaritone commented 2 years ago

Thanks for flagging! Regarding the default value, there are a few places where we check to see if the module has a .default property and use that instead. One recent example can be found here:

https://github.com/facebook/relay/commit/efd9d867ba7e4e984f27abe469a2e6c455bb1dc0

Would you be interested in working on a PR to get that working?

Regarding the relative module path, @tbezman has been working on a number of related issues, so he might have some ideas.

tbezman commented 2 years ago

Okay, so two issues here.

  1. Module .default stuff - Seems like we're just missing a check somewhere and is simple fix. Not sure if we have a system to consolidate these types of checks @captbaritone but that would pave the road so we don't have to do this kind of checking everywhere.
  2. Non Haste module resolution - @bigfootjon We're moving around the codebase trying to find all code paths that get affected by this kind of module resolution. We're then giving them a mechanism to support relative path imports so you don't have to think about that __generated__ stuff. This is kind of a lengthy process.

@captbaritone Have we thought about using the Github ticket system (I forget what it's called)? Might be nice to communicate with the public the progress we've made on this stuff and how much we have left.

Lalitha-Iyer commented 1 year ago

Is there a plan to fix the docs or re-open https://github.com/facebook/relay/pull/3970