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

fix: dynamic output property #205

Merged
merged 9 commits into from Aug 26, 2019
Merged

Conversation

jasonkuhrt
Copy link
Member

This change addresses the last thing remaining in the way before nexus-prisma can get back on the nexus mainline.

graphql-nexus/nexus-prisma#304

@tgriesser
Copy link
Member

This is close, but not quite the change we want - we want the ability to define both dynamic input methods & properties. I know what the issue is here, ran into it when adding tests for #200 - will open a little later this morning.

@jasonkuhrt
Copy link
Member Author

jasonkuhrt commented Aug 24, 2019

we want the ability to define both dynamic input methods & properties

Sounds good, but isn't that possible now, just depending on what factory returns? E.g.:

const block: any = {}

Object.defineProperty(block, "foo", {
  get: () => {
    return (): string => {
      return "bar"
    }
  },
})

Object.defineProperty(block, "nested", {
  get: () => {
    return {
      foo: (): string => {
        return "bar"
      },
    }
  },
})

console.log(block.foo()) // bar
console.log(block.nested.foo()) // bar

?

src/builder.ts Outdated Show resolved Hide resolved
src/builder.ts Outdated Show resolved Hide resolved
@tgriesser
Copy link
Member

This PR originally added dynamicObjectProperty - #178 the intention was that we should be explicit as to whether it's adding a property or a method rather than overloading it.

If we want to keep them as distinct concepts, I believe what's left is:

  1. adding the equivalent for "dynamic properties" on input types
  2. fixing quite a few places the symbols I added were incorrect since there weren't any tests and they needed to be cleaned up:

https://github.com/prisma/nexus/pull/200/files#diff-84231b52da5756993249faf30a25c7c3R34
https://github.com/prisma/nexus/pull/200/files#diff-629f635a7466a97bd69d407fe79be89aR150
https://github.com/prisma/nexus/pull/200/files#diff-cf514b96691f8608ebc06c9e05285e58R66
https://github.com/prisma/nexus/pull/200/files#diff-b2957fb214e0db032454663585fcdd3bR32
https://github.com/prisma/nexus/pull/200/files#diff-629f635a7466a97bd69d407fe79be89aR166

I had also written some tests for the "dynamic" method/property pieces on that plugin branch that might be useful to port over:

https://github.com/prisma/nexus/pull/200/files#diff-ce313718055422103750b425fdbcb1c2R1

Let me know what you think of this approach / if this clears up what I meant by ^.

@jasonkuhrt

This comment has been minimized.

@jasonkuhrt
Copy link
Member Author

jasonkuhrt commented Aug 25, 2019

Hey @tgriesser, you can ignore my previous comment. I started work on adding dynamicInputProperty but soon realized that this is, at least presently, not the minimal step to take. Doing so would be adding a feature. And nexus-prisma does not actually use dynamicInputProperty. It is actually interested in dynamicOutputProperty. It currently (via the nexus fork @prisma/nexus) uses dynamicOutputMethod. So two things need to happen that I can see:

  1. Update nexus-prisma to use dynamicOutputProperty
  2. Fix dynamicOutputProperty in nexus :)

This PR is now focused on point 2 above.

Via package link on local I was able to get tests passing on nexus-prisma with the changes herein.

@jasonkuhrt jasonkuhrt changed the title fix: adjust dynamic for nexus-prisma fix: dynamic output property Aug 25, 2019
@jasonkuhrt jasonkuhrt added the type/bug Something is not working the way it should label Aug 25, 2019
@jasonkuhrt jasonkuhrt self-assigned this Aug 25, 2019
@jasonkuhrt
Copy link
Member Author

I tried pulling in some of the tests from #200 but they were non-trivial as they depended upon numerous other additions in that PR not present here. Therefore stopping trying to port any over here now.

On account of this PR being now only additive and rather simple and clear, I am going to move ahead with it.

@jasonkuhrt jasonkuhrt merged commit 67d9025 into develop Aug 26, 2019
@jasonkuhrt jasonkuhrt deleted the fix/adjust-dynamic-for-nexus-prisma branch August 26, 2019 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is not working the way it should
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants