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

fix(datapath) must be equivalent for pull and push #6019

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jwallet
Copy link
Contributor

@jwallet jwallet commented May 17, 2024

This PR contains:

  • IMPROVED DOCS
  • IMPROVED TESTS
  • A BUGFIX
  • BREAKING

Describe the problem you have without this PR

If we define a custom dataPath for the graphQL replication, we end up setting a different datapath for the pull and push rep. Pull rep does not handle the data. prefix while push already handles it with result.data

I changed this to make both (pull and push) use the same function helper to extract the proper datapath if defined or not by the user.

Based on the unit tests, I found some that were testing only the pull replication and needed data as a prefix so I made the helper function the same as the current pull.dataPath, so it will request the user to add data. as a prefix, if you prefer the other way around, let me know. However, it makes sense if for any reason the default selector ['data', Object.keys(result.data)[0])] doesn't work because there is no data key returned from the server (exceptional edge case, maybe they used the customFetch property) and it breaks the default selector on Object.keys(result.data)

I also changed the typing of dataPath to accept string[] since getProperty also accepts it.

{
  dataPath: ['data', 'foo', 'bar']
}
  // or
{
  dataPath: 'data.foo.bar'
}

@pubkey
Copy link
Owner

pubkey commented May 17, 2024

Is this a breaking change?

@jwallet
Copy link
Contributor Author

jwallet commented May 17, 2024

Yes it is for the push replication. If someone used dataPath: 'pushNotifications' he will need to change it to 'data.pushNotifications'. Just like the pull dataPath.

@pubkey pubkey added the 16.0.0 label May 17, 2024
@pubkey
Copy link
Owner

pubkey commented May 18, 2024

Ok that makes it complicated. At the moment there is no major release planned. I added the 16.0.0 label so it will be merged, but this will take a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants