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(connectors): prefer wrappers over bind #2575

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Conversation

bobylito
Copy link
Contributor

Previously we were using bind, which will be applied at each init. This
is ok for one time mount, but since we can mount and unmount dynamically
now, it's a problem.

Previously we were using bind, which will be applied at each init. This
is ok for one time mount, but since we can mount and unmount dynamically
now, it's a problem.
@bobylito bobylito requested a review from iam4x November 13, 2017 17:08
@algobot
Copy link
Contributor

algobot commented Nov 13, 2017

Deploy preview ready!

Built with commit a5ed7d0

https://deploy-preview-2575--algolia-instantsearch.netlify.com

@Haroenv
Copy link
Contributor

Haroenv commented Nov 13, 2017

Hey 👋 looks like a good call. You still need to run prettier on this PR :)

@bobylito
Copy link
Contributor Author

Hey 👋 looks like a good call. You still need to run prettier on this PR :)

Well the only problem is that I haven't touched the files failing...

@bobylito
Copy link
Contributor Author

bobylito commented Nov 15, 2017

I need to fix the tests here :/ Apparently we're testing the internals on the widget side and of course this PR changes the internal.

@@ -158,13 +158,15 @@ export default function connectPriceRanges(renderFn, unmountFn) {
},

init({ helper, instantSearchInstance }) {
this._refine = this._refine.bind(this, helper);
this.refine = (...opts) => {
this._refine(helper, ...opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

refine only has two arguments, what about not spreading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed preferable to avoid spreading if we know the number of arguments 👍

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.

I think it's good, but I don't really understand the reasoning how this can be faster. .bind only happens when init is called, while now the currying happens every time refine is called

@bobylito
Copy link
Contributor Author

Happy to take 5 minutes and go over the code to explain if you want @Haroenv

@bobylito bobylito merged commit f8e0e00 into feat/2.3 Nov 16, 2017
@bobylito bobylito deleted the feat/2.3-fix-hot-remount branch November 16, 2017 14:25
bobylito pushed a commit that referenced this pull request Nov 16, 2017
<a name=2.3.0-beta.6></a>
# [2.3.0-beta.6](v2.3.0-beta.5...v2.3.0-beta.6) (2017-11-16)

### Bug Fixes

* **connectors:** prefer wrappers over bind ([#2575](#2575)) ([f8e0e00](f8e0e00))
bobylito pushed a commit that referenced this pull request Nov 20, 2017
<a name=2.3.0-beta.7></a>
# [2.3.0-beta.7](v2.2.5...v2.3.0-beta.7) (2017-11-20)

<a name=2.3.0-beta.6></a>
# [2.3.0-beta.6](v2.3.0-beta.5...v2.3.0-beta.6) (2017-11-16)

### Bug Fixes

* **connectors:** prefer wrappers over bind ([#2575](#2575)) ([f8e0e00](f8e0e00))

<a name=2.3.0-beta.5></a>
# [2.3.0-beta.5](v2.2.4...v2.3.0-beta.5) (2017-11-16)

### Features

* **core:** InstantSearch hot remove/add widgets ([#2384](#2384)) ([cfc1710](cfc1710))

<a name=2.3.0-beta.3></a>
# [2.3.0-beta.3](v2.3.0-beta.2...v2.3.0-beta.3) (2017-10-25)

### Bug Fixes

* **connectors:** export  as well ([0b78d75](0b78d75))

### Features

* **refinementList:** add escapeFacetHits parameter ([#2507](#2507)) ([9b1b7ee](9b1b7ee))

<a name=2.3.0-beta.2></a>
# [2.3.0-beta.2](v2.2.1...v2.3.0-beta.2) (2017-10-24)

### Bug Fixes

* **connectHierarchicalMenu:** do not return if facet not set ([#2521](#2521)) ([26e99fb](26e99fb))

### Features

* **breadcrumb:** Add the breadcrumb widget ([#2451](#2451)) ([11d78f0](11d78f0)), closes [#2299](#2299)
* **connectRange:** round the range based on precision ([#2498](#2498)) ([d4df45d](d4df45d))
* **rangeInput:** add rangeInput widget ([#2440](#2440)) ([7916d16](7916d16))
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.

4 participants