-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
WIP: feat: Add migration context to enable GraphiQL refactoring #1380
WIP: feat: Add migration context to enable GraphiQL refactoring #1380
Conversation
@@ -113,7 +113,7 @@ export const schemaReducer: SchemaReducer = ( | |||
*/ | |||
|
|||
export type SchemaProviderProps = { | |||
config: SchemaConfig; | |||
config?: SchemaConfig; |
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.
This already provided a default so it seems it'd be fine to have as optional.
export const GraphiQL: React.FC<GraphiQLProps> & | ||
GraphiQLStaticProperties = props => { | ||
return ( | ||
<MigrationContextProvider> | ||
<GraphiQLInternals {...props} /> | ||
</MigrationContextProvider> | ||
); | ||
}; |
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.
I know this and the changes that follow are a bit ugly. I'm trying to preserve the api without requiring an extra wrapper which required some... creativity.
interface GraphiQLStaticProperties { | ||
formatResult: (result: any) => string; | ||
formatError: (rawError: Error) => string; | ||
Logo: typeof GraphiQLLogo; | ||
Toolbar: typeof GraphiQLToolbar; | ||
Footer: typeof GraphiQLFooter; | ||
QueryEditor: typeof QueryEditor; | ||
VariableEditor: typeof VariableEditor; | ||
ResultViewer: typeof ResultViewer; | ||
Button: typeof ToolbarButton; | ||
ToolbarButton: typeof ToolbarButton; | ||
Group: typeof ToolbarGroup; | ||
Menu: typeof ToolbarMenu; | ||
MenuItem: typeof ToolbarMenuItem; | ||
} |
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.
Gotta be a better way to do this.
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.
we're about to drop this interface for 1.0.0 so don't worry about it too much:
#1165
Note that the context isn't as of yet actually being used. To access the schema provider you'd use |
@@ -1266,6 +1296,8 @@ export class GraphiQL extends React.Component<GraphiQLProps, GraphiQLState> { | |||
}; | |||
} | |||
|
|||
GraphiQLInternals.contextType = MigrationContext; |
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.
This is the magic that enables this.context.schema
inside the class component.
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.
oh yes, i remember it used to work this way with the old context API too, amazing how little the class component interface changed
* Initial migration context wrapper * Pull out other static properties * Fix refs bug by properly accessing current
This PR in essence wraps the entirety of GraphiQL in an aggregated
context provider to which other context providers can be attached.
The short term goal of this is to separate out what is now GraphiQL state
into separate, smaller contexts which can be hooked into my plugins, etc
Being as we must ensure the provider is wrapped around GraphiQL I had to make
a facade component to do the wrapping and proxy props to the actual class.