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

Deep extend when adding global query params in ufTable #1114

Closed
wants to merge 1 commit into from

Conversation

alexweissman
Copy link
Member

At the moment if you were to use addParams to set a global filter or sort for a table, it would overwrite all other filters and sorts applied to the table. It doesn't seem like this was a deliberate design choice when addParams was introduced in 3159224. By using a deep extend instead, we can set global filters without overriding any sorts/filters set by the user. It does technically change the behavior, but I don't see a valid use case for the way it currently behaves.

At the moment if you were to use `addParams` to set a global `filter` or `sort` for a table, it would overwrite all other filters and sorts applied to the table. It doesn't seem like this was a deliberate design choice when `addParams` was introduced in 3159224. By using a deep extend instead, we can set global filters without overriding any sorts/filters set by the user. It does technically change the behavior, but I don't see a valid use case for the way it currently behaves.
@Silic0nS0ldier
Copy link
Member

Code looks fine, but I can't approve this. Merging options can be a deceptively complex process, as certain combinations of merged configurations can end up being invalid (which ends up requiring people passing configurations in having to explicitly specify more configuration options).

To properly review this I've need to first figure out the shape of the configuration option and what those options are.

@alexweissman
Copy link
Member Author

@Silic0nS0ldier In general agree, but in this case the parameters are only merged into a very specific portion of the config options. Namely, the ajaxObject.data key which is empty by default. See https://mottie.github.io/tablesorter/docs/example-pager-ajax.html and https://api.jquery.com/jQuery.ajax/#jQuery-ajax-settings. So, it shouldn't be possible to touch any of the other Tablesorter or ufTable parameters through this setting.

Copy link
Member

@Silic0nS0ldier Silic0nS0ldier left a comment

Choose a reason for hiding this comment

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

Alright. LGTM. Just need to make sure the changelog mentions what has changed (to address the remote change someone is impacted).

@lcharette lcharette modified the milestones: 4.5.0, 4.4.4 Nov 13, 2020
@lcharette lcharette changed the base branch from develop to hotfix November 14, 2020 22:03
@lcharette lcharette changed the base branch from hotfix to develop November 14, 2020 22:03
@lcharette
Copy link
Member

Since the base got messed up, I merged it manually in Hotfix: 3310530

@lcharette lcharette closed this Nov 14, 2020
@lcharette lcharette deleted the alexweissman-patch-2 branch April 22, 2021 00:31
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