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

remove _timestamp properties #3055

Open
gvwilson opened this issue Oct 29, 2024 · 9 comments
Open

remove _timestamp properties #3055

gvwilson opened this issue Oct 29, 2024 · 9 comments
Assignees
Labels
feature something new P1 needed for current cycle

Comments

@gvwilson
Copy link
Contributor

The _timestamp properties have been essentially useless since callback_context was introduced - they should be removed. cc @alexcjohnson @AnnMarieW

@gvwilson gvwilson added feature something new P1 needed for current cycle labels Oct 29, 2024
@AnnMarieW
Copy link
Collaborator

I agree - but do you think it's still important for other languages? Not sure if that's important if they are no longer being supported.

Here is an R example

Also is it necessary in the DataTable?

@alexcjohnson
Copy link
Collaborator

IMO since this is a major version bump of Dash, if other languages want to upgrade their dependencies on the main components it’s reasonable to ask them to support the callback_context pattern as well.

@alexcjohnson
Copy link
Collaborator

Yes, I think we can remove this from DataTable, also html:

'n_clicks_timestamp': PropTypes.number,

I have a vague recollection of there being a reason to keep it for Store, but if we don’t have an example of this then probably that too is no longer applicable.

modified_timestamp: PropTypes.number,

@gvwilson
Copy link
Contributor Author

Thanks @alexcjohnson and @AnnMarieW - @T4rk1n, please pull it out. (I love deleting code in the morning...)

@T4rk1n
Copy link
Contributor

T4rk1n commented Oct 31, 2024

I have a vague recollection of there being a reason to keep it for Store

I think that is what we have that can trigger a callback without an input on the data itself. Like you may want to update some last saved element but don't need the data to be transfered.

@gvwilson
Copy link
Contributor Author

gvwilson commented Nov 1, 2024

@T4rk1n @alexcjohnson @AnnMarieW please thumbs-up if you believe we can remove _timestamp properties in the upcoming release.

@AnnMarieW
Copy link
Collaborator

Yes - all except Store per comments above

@alexcjohnson
Copy link
Collaborator

Does that same argument apply to DataTable data_timestamp? Otherwise the props are all small so no reason not to fully transfer them, seems like this is just a performance hack for very large props

@alexcjohnson
Copy link
Collaborator

TBH though I’m not sure that’s a good enough reason to keep these props around. It’s a very niche case (how often do you really care that something changed but not care what the new value is?), and the same could be accomplished with a small clientside callback bound to the full prop outputting to another prop (an extra store just for this purpose, for example) then you use that as the input to your serverside callback that doesn’t need the full data transferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

No branches or pull requests

4 participants