Skip to content

DO NOT MERGE: alternative state management RFC #2906

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

Closed
wants to merge 13 commits into from

Conversation

jonathanawesome
Copy link
Collaborator

Tracking issue #2904

@changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2022

⚠️ No Changeset found

Latest commit: 1d360ef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@acao acao requested review from dimaMachina, thomasheyenbrock and imolorhe and removed request for imolorhe, dimaMachina and thomasheyenbrock November 21, 2022 07:27
@acao
Copy link
Member

acao commented Nov 21, 2022

Decided not to invite y’all as proper reviewers yet because it’s still WIP but @B2o5T and @thomasheyenbrock we feel that we can rework state management in a 2.x refactor to make the rest of the planned features and plugin API easier to build, and for more intelligent & efficient component rendering - what do you think?

new Promise(res => setTimeout(res, timeout));

const longDescription = `
The \`longDescriptionType\` field on the \`Test\` type has a long, verbose, description to test inline field docs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The \`longDescriptionType\` field on the \`Test\` type has a long, verbose, description to test inline field docs.
The \`longDescriptionType\` field on the \`Test\` type has a long, verbose description to test inline field docs.

Copy link
Collaborator

@thomasheyenbrock thomasheyenbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea of moving the doc explorer into its own plugin package! Aligns nicely with all the other plugins, both existing ones and future ones.

My main question here is if we can make this backwards compatible with the existing hooks from v2. In other words, can we keep exporting the ExplorerContext and the useExplorerContext hook while moving all the actual state management from the ExplorerContextProvider component into zustand?

I guess it would not require a major version bump for graphiql either way, but we should decide if we're fine with breaking stuff for folks that use @graphiql/react directly.

@acao
Copy link
Member

acao commented Nov 26, 2022

@thomasheyenbrock yes this was part of the original plugin proposal in #1689 , the idea was that the plugin API should be able to support doc explorer, query history, etc as plugins!

also, @graphiql/react is still in 0.x state, so direct consumers will expect breaking changes over minor versions as per semver until we feel we have a stable API for 1.x (maybe that can be the goal for 3.0.x).

@@ -0,0 +1,4354 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this lock file should be removed since you added laddle to workspace

@@ -0,0 +1,22 @@
{
"extends": [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"extends": [
"root": true,
"extends": [

this config do nothing, without root: true always root eslint config is taken in priority

<div className="graphiql-doc-explorer-header-content">
{prevName && (
<button
// href="#"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// href="#"

@@ -0,0 +1,19 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
{
"root": true,

same here

@@ -0,0 +1,19 @@
{
"extends": [
"plugin:prettier/recommended",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"plugin:prettier/recommended",

I don't like eslint-plugin-pretter since he make a lot of noise, anyway prettier check action is enabled in ci

@acao acao marked this pull request as draft January 22, 2023 15:25
@dimaMachina
Copy link
Collaborator

dimaMachina commented May 13, 2025

replaced by #3946, tracking progress in #3874

@dimaMachina dimaMachina deleted the rfc/migrate-from-react-context-to-zustand branch May 13, 2025 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants