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(js): support updating Element options #777

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

salomvary
Copy link
Contributor

I've not opened a separate Issue for this because showcasing the problem seemed easier in the form of a failing test case. I hope that's OK.

Before this fix, a call to update() like this:

update({panelContainer: document.createElement('div')})

would throw the following error:

TypeError: panelContainer.contains is not a function

The reason lies in an incorrect object deep merge implementation where
object entry values of Element type are merged like plain objects
instead of being replaced, resulting in broken non-Element entries.

This change fixes the issue with HTML Elements (Nodes of any kind to be
more precise) but it might be worth considering replacing the current
mergeDeep implementation with something more robust and future proof.
Deep merging of objects surprisingly hard to get right, see the
implementation in Lodash for example:
https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L3633

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 19, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@salomvary salomvary force-pushed the update-panelContainer branch from a7782a6 to 7c6fa67 Compare October 19, 2021 06:07
@salomvary salomvary changed the title Fix updating Element options fix(autocomplete-js): updating Element options Oct 19, 2021
Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

Great—thank you very much for the contribution!

@francoischalifour francoischalifour changed the title fix(autocomplete-js): updating Element options fix(js): support updating Element options Oct 19, 2021
Before this fix, a call to update() like this:

  update({panelContainer: document.createElement('div')})

would throw the following error:

  TypeError: panelContainer.contains is not a function

The reason lies in an incorrect object deep merge implementation where
object entry values of Element type are merged like plain objects
instead of being replaced, resulting in broken non-Element entries.

This change fixes the issue with HTML Elements (Nodes of any kind to be
more precise) but it might be worth considering replacing the current
mergeDeep implementation with something more robust and future proof.
Deep merging of objects surprisingly hard to get right, see the
implementation in Lodash for example:
https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L3633
@salomvary salomvary force-pushed the update-panelContainer branch from bb278ad to 285b3e8 Compare October 20, 2021 16:50
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

thanks!

@francoischalifour francoischalifour merged commit fe684b3 into algolia:next Oct 21, 2021
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