Skip to content

WIP: feat: Add migration context to enable GraphiQL refactoring#1380

Merged
acao merged 3 commits intographql:feat/use-context-hooksfrom
just-be-dev:feat/use-context-hooks
Feb 28, 2020
Merged

WIP: feat: Add migration context to enable GraphiQL refactoring#1380
acao merged 3 commits intographql:feat/use-context-hooksfrom
just-be-dev:feat/use-context-hooks

Conversation

@just-be-dev
Copy link
Copy Markdown
Contributor

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.

@just-be-dev just-be-dev changed the base branch from master to feat/use-context-hooks February 28, 2020 06:31

export type SchemaProviderProps = {
config: SchemaConfig;
config?: SchemaConfig;
Copy link
Copy Markdown
Contributor Author

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.

Comment on lines +141 to +148
export const GraphiQL: React.FC<GraphiQLProps> &
GraphiQLStaticProperties = props => {
return (
<MigrationContextProvider>
<GraphiQLInternals {...props} />
</MigrationContextProvider>
);
};
Copy link
Copy Markdown
Contributor Author

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.

Comment on lines +150 to +164
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;
}
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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

@just-be-dev
Copy link
Copy Markdown
Contributor Author

Note that the context isn't as of yet actually being used. To access the schema provider you'd use this.context.schema inside the GraphiQLInternals class component.

};
}

GraphiQLInternals.contextType = MigrationContext;
Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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

@acao acao changed the title WIP: Add migration context to enable GraphiQL refactoring WIP: feat: Add migration context to enable GraphiQL refactoring Feb 28, 2020
@acao acao merged commit 1a37fe6 into graphql:feat/use-context-hooks Feb 28, 2020
@acao acao added this to the GraphiQL-1.0.0-beta milestone Mar 14, 2020
acao pushed a commit that referenced this pull request Mar 16, 2020
* Initial migration context wrapper
* Pull out other static properties
* Fix refs bug by properly accessing current
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

Successfully merging this pull request may close these issues.

2 participants