-
Notifications
You must be signed in to change notification settings - Fork 24
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
V3 + updates #92
V3 + updates #92
Conversation
|
||
// directive tests | ||
|
||
// TODO: implement directive tests to test directive behavior between imports, etc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we might need to write tests here and/or make minor tweaks at schema construction time to make sure directives work the way we think/want when there is a tree of components (ie. imports)
- how do they stack if 2 different directives are added to the same field on the same type defined in 2 components, one who imports another
- conflicts - both the importing and importee components define and use a directive of the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlivings I am reviewing this pr and it looks good to me. I just need to do some testing on the polaris-api-gateway managed federation stack. I am working on that now. Just wanted to update you, two to three more days and I should be good to go on this. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlivings I have completed my review and testing and this is ok to be merged as a breaking change. Thank you for this, it def fixes some of the issues we were experiencing with federation and wrapResolverRoot. :)
Pulled in Brian's change and added back memoize. Also intending to resolve conflicts.