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

[NOT MERGE] Repro and "fix" #362 #373

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dragoshomner
Copy link
Contributor

This PR is a similar repro of the #362 issue and it shows how this problem can be fixed. However, it still should work with the directives + it still doesn't work in 1JS, so this issue might be related

@alloy
Copy link
Member

alloy commented Oct 30, 2023

@dragoshomner Can you please provide some inline commentary about the changes and what/how they work?

@dragoshomner
Copy link
Contributor Author

@dragoshomner Can you please provide some inline commentary about the changes and what/how they work?

@alloy I added a couple of commnets to make it more clear what I did. For short, I tried to simulate what we have in People App (PA). There, we a similar scenario, where we keep the variables passed to the query (sortBy and filterBy) in some React states, and we change their values when we receive an event (in this example is refetch function that sets the state).

As I mentioned in the chat, when we change any state, the app crashes because data is undefined in useFragment. We had the same issue in the example app as well, but I was able to "fix" it by removing some directives (@argumentDefinitions, @includes) and setting fist: 5 in examples/apollo-watch-fragments/src/TodoList.tsx. However, it's very fragile (as you see @include directive breaks it). I'm not able to fully understand what's happening, but this fragility increases exponentially in our app and it's impossible for me to figure out why. That's what we did in PA: https://office.visualstudio.com/Office/_git/1JS/pullrequest/2360178

The goal, as I mentioned during the Nova sync, is to migrate to duct-tape + pagination step-by-step, to reduce the risk. The first one is to replace the old nova compiler to duct-tape compiler in 1JS and make it work in TMP. I suppose that the old code should be compatible with both compilers (the only important change being that we removed the fragment interpolation). Then, the next step would be to use useRefetchableFragment and usePaginationFragment instead of the basic useFragment, but we are blocked on the first step.

Hope it helps for more context :)

@sjwilczynski
Copy link
Contributor

@dragoshomner should we close this? Or was there something that hasn't been fixed yet?

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.

3 participants