[WIP] Use normal store instead of React state with disabled location sync

[btbenedi]

This change is to continue using the normal React Admin store (local storage by default) when disableSyncWithLocation is used, this enables filters, sort, and pagination to be persisted across navigation and component mount/unmount.

In order for this behavior to work, the storeKey default value is left blank when disableSyncWithLocation is true.

storeKey is then checked (if blank) in changeParams, if blank, old behavior remains, uses localParams that do not persist, if storeKey is set, then it uses store as normal.

[WIP] [TODO] Documentation and tests updated to reflect this additional behavior

I feel this meets the design choice that filter values are persisted in application state as stated at the bottom of this section even if you need to not sync those values with the location due to have multiple List components on one page.

Implements #9100

[slax57]

Thank you for your contribution.

Indeed, your code introduces a breaking change. We cannot afford to make breaking changes for react-admin v4 (it would not respect semantic versioning).

You need to find a way to make your feature backwards compatible, e.g. by introducing new props.
In this case, since it's a new feature, you will need:

• to target the next branch
• to provide automated tests and stories to prove it works
• to update the documentation to explain how it works

If there are no way to make your feature backwards compatible, then it should be kept for react-admin v5 (which has no dedicated branch yet).

Lastly, I don't fully agree with you on that part:

My only thought on that is if someone used disableSyncWithLocation to kill all filter persistence. In that case, I would suggest those applications to use memoryStore for their app's store prop.

Disabling the persistence for one List is not the same as disabling the persistence for all settings that are stored in the Store.
Besides it would require all users currently using disableSyncWithLocation to change their code if they update to the latest version of RA, which is something we don't want.

[btbenedi]

Thank you for your contribution.

Indeed, your code introduces a breaking change. We cannot afford to make breaking changes for react-admin v4 (it would not respect semantic versioning).

You need to find a way to make your feature backwards compatible, e.g. by introducing new props. In this case, since it's a new feature, you will need:

• to target the next branch
• to provide automated tests and stories to prove it works
• to update the documentation to explain how it works

If there are no way to make your feature backwards compatible, then it should be kept for react-admin v5 (which has no dedicated branch yet).

Lastly, I don't fully agree with you on that part:

My only thought on that is if someone used disableSyncWithLocation to kill all filter persistence. In that case, I would suggest those applications to use memoryStore for their app's store prop.

Disabling the persistence for one List is not the same as disabling the persistence for all settings that are stored in the Store. Besides it would require all users currently using disableSyncWithLocation to change their code if they update to the latest version of RA, which is something we don't want.

I definitely see your perspective here and agreed on breaking change with that context.

I will think about this and see if I can come up with a clean way to enable this in v4.

I am curious, is the intention of disableSyncWithLocation suppose to kill the persistence of filters? Just from a mindset point of view.

I started using React Admin in v4, so I don't have the history on it, but my understanding from commit history is this:

• v3 and earlier = list params would originally persist always with Redux store?? (completely unknown here, guessing based on basic knowledge of old versions)
• v3 = syncWithLocation prop was added to keep URL in sync with params, and in turn, added localParams via React state to stop syncing the Redux store, seen in this commit
• v4 = Redux removed and store (local storage default) as we know it now came to be, and at this time the prop was inverted from syncWithLocation to disableSyncWithLocation

Would it make sense for v4 to add something like persistParams that starts (but remains backwards compatible) separating param persistence from the location sync feature? Really just struggling on the naming of it

[slax57]

@btbenedi You may be correct about the history of this design decision, honestly I don't know.

I agree it would be nice to have the ability to disable the syncing with location, without disabling the parameters persistence in the Store.

But it needs to be backwards compatible and we want to avoid adding too much complexity.

Here's what I suggest:
If the List has disableSyncWithLocation to true:

• if the List has no storeKey, or if the storeKey is empty: the params will be stored in an internal state, and won't be persisted
• if the List has storeKey set to a non-empty String: the params will be stored using the Store under the provided storeKey

What do you think? Do you think you could try to implement this?

[btbenedi]

@slax57 I think I can manage that, and that is a solid idea, better than adding yet another prop

I'll take a shot at it, I tried unsuccessfully last week to get the repo running locally, but this change is small enough I can probably get away with just using the PR builds

[slax57]

@btbenedi thank you for your answer, and for giving it a shot 🙂

Some important additional info though: as stated in the contribution guidelines we will ask for tests and stories to prove the new feature is working (and make sure it stays that way in the future). So relying solely on existing tests won't be enough, you'll have to write new ones.
I can maybe help you set up your workspace if you are having difficulties.

Also, since this will be a new feature, your PR should target the next branch.

Thanks a lot!

[btbenedi]

@slax57 started looking at this, storeKey has a default set to ${resource}.listParams so storeKey will never be empty, unless the user specifically passes an empty string, which feels less backwards compatible

Thoughts?

[slax57]

@btbenedi If I'm not mistaken, you can safely change this, because the storeKey is only read when disableSyncWithLocation is set to false. The change I'm suggesting should only happen when disableSyncWithLocation is set to true. So you can change the code to set the current default value when disableSyncWithLocation !== true, and let the value be empty when disableSyncWithLocation === true.

[btbenedi]

@slax57 ahh, that makes sense, i'll get that set, and then start in on the tests/docs side of this PR

[btbenedi]

@slax57 default value updated in code, and original PR description updated to reflect this change/behavior

Will hopefully get to tests/documentation next week, still having issues with local environment, I think it's mainly due to node/npm versions, but haven't dug too much deeper.

[slax57]

@btbenedi Thanks.

Our team is a bit busy ATM, so don't panic if we don't review your PR right away. But it is in our to-do list and we'll do our best to be helpful in the meantime 🙂

Regarding the versions, if that can help, here are mine:

$> node -v
v16.17.0
$> yarn -v
3.1.1
$> npm -v
9.1.2

(btw I recommend using yarn for this project since it is the tool we use internally)

[fzaninotto]

Please update your PR with tests and documentation, and fix the linter warnings.

[btbenedi]

Please update your PR with tests and documentation, and fix the linter warnings.

as mentioned in the enhancement, real work has gotten in the way of me getting to fixing the known issues with tests/docs/linter

If someone else wants to fork this and complete it, they are more than welcome to, but at this time, I cannot guarantee I will be able to complete this work myself.

Feel free to close this PR if applicable.

[fzaninotto]

Thanks for the update. I'll close this PR as it's good for our morale to have a low number of open PRs ;) Feel free to reopen it when you have time to work on it.