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

Remote data refresh support #7

Merged
merged 5 commits into from
Oct 4, 2024
Merged

Conversation

isaacabraham
Copy link
Member

@isaacabraham isaacabraham commented Sep 8, 2024

This PR makes a small but important change to the RemoteData<'T> type:

| Loading

becomes

| Loading of 'T option

this is very useful for refresh scenarios, whereby you load data and then need to load it again later. Without this, you "lose" the existing data whilst refreshing, which can cause e.g. flickering on screen. There is the RefreshableRemoteData type but it is somewhat limited - it has no built-in functionality such as map etc. and is somehow awkward to use. Now, if you just want to load data up-front once, you would have Loading None during the load. If you're refreshing and want to keep the current data, then you would put it into Loading (Some xxx).

I've added extra /// comments to aid this change, and adding some an extra member IsRefreshing, and updated the existing members to act appropriately on Loading data e.g. map, defaultValue, hasData etc.

Thoughts?

Copy link
Member

@mattgallagher92 mattgallagher92 left a comment

Choose a reason for hiding this comment

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

I like the premise of the change 🙂

I'd prefer not to break the existing API for no reason, so recommend that we reinstate the HasStarted/hasStarted functions.

I have a few other optional suggetions.

src/SAFE.Client/SAFE.fs Show resolved Hide resolved
src/SAFE.Client/SAFE.fs Show resolved Hide resolved
src/SAFE.Client/SAFE.fs Outdated Show resolved Hide resolved
src/SAFE.Client/SAFE.fs Show resolved Hide resolved
src/SAFE.Client/SAFE.fs Outdated Show resolved Hide resolved
src/SAFE.Client/SAFE.fs Outdated Show resolved Hide resolved
@isaacabraham
Copy link
Member Author

@mattgallagher92 I think that your comments have all been addressed - thank you for them :-) @Larocceau thanks for helping out with this.

@Larocceau Larocceau merged commit 16897dc into main Oct 4, 2024
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