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

getGlobalState loses sync with useGlobalState after 0.17 #33

Closed
Slessi opened this issue Jan 15, 2020 · 8 comments · Fixed by #34
Closed

getGlobalState loses sync with useGlobalState after 0.17 #33

Slessi opened this issue Jan 15, 2020 · 8 comments · Fixed by #34

Comments

@Slessi
Copy link

Slessi commented Jan 15, 2020

After updating to 0.16 -> 0.17, getGlobalState returns the old state value after a change. useGlobalState seems fine

@dai-shi
Copy link
Owner

dai-shi commented Jan 15, 2020

Thanks for reporting. I will take a look. It can be tough.

dai-shi added a commit that referenced this issue Jan 15, 2020
@dai-shi dai-shi mentioned this issue Jan 15, 2020
@dai-shi
Copy link
Owner

dai-shi commented Jan 15, 2020

@Slessi I tried to make a failing test in #34, but it doesn't fail. Any insight? Is it easy for you to reproduce the problem?

@dai-shi
Copy link
Owner

dai-shi commented Jan 15, 2020

I think I get it now. 8565e67
You are likely to getGlobalState in callback or where you update the value.
Yeah, there's a delay. In that sense, v0.17 has a breaking change.

Would you tell me your use case? I'd like to see if/how we can solve or mitigate it.

@Slessi
Copy link
Author

Slessi commented Jan 15, 2020

@dai-shi has been trimmed down a bit, but basically:

  1. useGlobalState updates due to a change
  2. AppLayout is loaded
  3. useQuery hook runs but the client still has old value in state
  4. API Error
const authMiddleware = new ApolloLink((operation, forward) => {
  // This sets the headers for my API queries
  operation.setContext({
    headers: getAuthHeaders(getGlobalState('authUser')),
  });

  if (!forward) {
    return null;
  }

  return forward(operation);
});

// This is the client used by queries, with headers
const client = new ApolloClient({  link: ApolloLink.from([ authMiddleware ]) });

const LayoutSwitcher = () => {
  const [authUser] = useGlobalState('authUser');

  // This decides what view to render based on if im authenticated or not
  return authUser !== null ? <AppLayout /> : <AuthLayout />;
};

const AppLayout = () => {
  // This uses apollo client, and query requires the authentication it provides
  // However, I am thrown 401 Unauthorized by API (in 0.17 only)
  const { data } = useQuery(gql`some query`);

  return <div>stuff</div>;
};

@dai-shi
Copy link
Owner

dai-shi commented Jan 15, 2020

Thanks! I totally understand how this is happening. Please give me some time for this. This is tricky...

@dai-shi
Copy link
Owner

dai-shi commented Jan 16, 2020

@Slessi So, here's bad news for your use case.

First of all, it's all fine with React Sync Mode and v0.16.

v0.17 is for React Concurrent Mode. #31
state branching is a behavior in CM, where React renders a component tree concurrently.
With state branching, the state value will not be fixed until React committed it.

Now, I can only think of two options (apart from using v0.16) for your use case.

  1. render AppLayout after the state value is committed.
  2. do not use state value for authMiddleware. (use a variable outside React as client is so.)

The first option doesn't sound nice, because it's against CM and the code will be hacky.
The second option is regrettable, because it means this lib can't help such use cases.

I wonder how Apollo Client will deal with CM, but I don't think it will accept apollo link config from state. So, it's probably the right way to store auth information outside React. Not sure, will see.

I will keep think about any workaround to bridge this gap.
It might be interesting to have a function that returns a promise which resolves the committed value. (But, it doesn't work nicely for your use case. It's like option 1 unessl useQuery supports CM.)

Hope it explains to some extent. Questions are welcome.

@dai-shi
Copy link
Owner

dai-shi commented Jan 23, 2020

With #35, the new release is v1.0.0-alpha.1.
This version allows to opt-in CM support,
so if you use it normally, it should work like v0.16.0.

Note the new version includes a BREAKING CHANGE and there is no GlobalStateProvider.

@Slessi Hope it works with your use case.

@dai-shi
Copy link
Owner

dai-shi commented Jan 24, 2020

#34 reopened.

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 a pull request may close this issue.

2 participants