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

RFC: Primitive Proxies #1516

Open
vicary opened this issue Feb 19, 2023 · 9 comments
Open

RFC: Primitive Proxies #1516

vicary opened this issue Feb 19, 2023 · 9 comments
Assignees

Comments

@vicary
Copy link
Member

vicary commented Feb 19, 2023

Summary

Just had a rough idea of primitive proxies while I am refactoring, PoC works so far.

This, albeit a bit hacky, allows us to add custom methods into scalars while retaining all existing primitive methods.

  1. PoC: React
  2. PoC: Vue

This affects all existing scalars, please let us know what you think and whether you have conflicting implementations.

Intended use cases

Directives

(This is only an example, API not decided yet)

function Component() {
  const [showName, setShowName] = useState(false);
  
  return <>
    {user.name.$skip({ if: showName })}
  </>
}

Custom GraphQL Scalars

Besides directives, I am also planning to treat this as an extension point for custom GraphQL scalars (graphql-scalars) in the future.

Questions

  1. null and undefined
  2. equality
@vicary vicary pinned this issue Feb 19, 2023
@vicary vicary self-assigned this Feb 19, 2023
@vicary
Copy link
Member Author

vicary commented Feb 19, 2023

@redbar0n You may be interested on this one.

@redbar0n
Copy link
Contributor

Proxies sounds good. The implementation goes over my head, though. Will probably have more opinions and suggestions as far as the API goes.

user.name.$skip({ if: showName })

looks particularly neat! Compared to previous suggestion in #448 :

user.name({ '@skip': { if: showName } }) // from the suggestion in issue 448 which was: u.member({ '@skip': { if: true } })

But what about duplicate directives on the same field, which the GraphQL spec allows, as you mentioned previously. So this previous suggestion:

u.name({ '@skip': { if: skipName }, @include’: { if: true } }),

Could maybe be chained like this instead?

user.name.$skip({ if: showName }).$include({ if: includeName })

Btw, the React PoC gives:

Error in /turbo_modules/[email protected]/cjs/react-dom.development.js (14757:9)
Objects are not valid as a React child (found: object with keys {}).
If you meant to render a collection of children, use an array instead.

@vicary
Copy link
Member Author

vicary commented Feb 19, 2023

@redbar0n Sorry I forgot to fork again before I start testing for nullables, the React PoC should be working now.

Directives API should be easy if we have primitives sorted out, the main concern is whether people find primitive wrapper classes surprising. i.e. query.me.name === "John Doe" // -> false and you have to call .valueOf() like this query.me.username.valueOf() === "John" // -> true.

This problem could be particularly bad for booleans, null and undefined.

@redbar0n
Copy link
Contributor

redbar0n commented Feb 19, 2023

Could this work?

query.me.username.value
query.me.username.directives

I feel like I've seen similar type of API's in JS before, maybe in data or request objects, or maybe on the frontend with stuff like document.forms["username"]["user"].value

@vicary
Copy link
Member Author

vicary commented Feb 21, 2023

Vanilla conventions such as .valueOf(), .toString() and Symbol.toPrimitive would be better, it helps type coercion.

I like the chaining API for directives, $skip({ if: true }) just feels more like @skip(if: true) in GraphQL.

I am wondering if users would find primitive objects surprising, consider the following example:

function Component() {
  return (
    <>
      { // This will ALWAYS render
        query.me.isVerified && <h1>Verified</h1>
      }

      { // This renders correctly
        query.me.isVerified.valueOf() && <h1>Verified</h1>
      }
    </>
  );
}

The Verified text would always render because isVerified is the object Boolean(false) instead of primitive false.

@redbar0n
Copy link
Contributor

redbar0n commented Feb 28, 2023

Just a thought: If the underlying implementation is a proxy object, then maybe it is best to use an object as the API instead of chaining, to reduce the impedance mismatch?

Inspired by reading: "Passing a shared value without .value getter causes Reanimated to watch for shared value changes." https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/animations/#animations-in-inline-styles

@vicary
Copy link
Member Author

vicary commented Mar 1, 2023

@redbar0n How would you like to add directives with object as the API?

@redbar0n
Copy link
Contributor

redbar0n commented Mar 7, 2023

@vicary ah, I see how it was ambiguous. I was thinking about passing in an object to a function, instead of chaining. But that only works for setting the proxy state, not retrieving it.

@redbar0n
Copy link
Contributor

redbar0n commented Mar 7, 2023

I am wondering if users would find primitive objects surprising, consider the following example:

Yes, I think so. I would for sure.

I'm coming back to my suggestion here #448 (comment) :

Most important presumption here is that all fields would always have to be functions (which again returns the value).


I think that would solve it, because then you'd have full control over what the function returns. The only thing the user would have to remember is to always end the chaining with a () e.g. query.me.isVerified()

@vicary vicary unpinned this issue May 4, 2024
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

No branches or pull requests

2 participants