-
Notifications
You must be signed in to change notification settings - Fork 516
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(types): improve InstantSearch types #3651
Conversation
Deploy preview for instantsearchjs ready! Built with commit b7b7869 |
@@ -59,7 +64,11 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/query-rule- | |||
container: document.createElement('div'), | |||
}); | |||
|
|||
widget.init({ helper, state: helper.state, instantSearchInstance: {} }); | |||
widget.init!({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's no guarantee that the init
or render
method of the widget exists, we need to force them with !
in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to solve this using conditional types to get the correct return type from queryRuleCustomData
? (my fear is that the user get the same issue when calling widget.init
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried some stuff like conditional types with @samouss but each solution brought other complications. @samouss has more input than me on this.
I don't think users have to manipulate widget.init
except in tests as we do.
Another solution would be to enforce init
, render
and dispose
from now on. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I still lack context on the lib to see the impact on enforcing these methods.
But I would be very interested to see what complications you encountered with @samouss with conditional types (for my own curiosity 😉 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would be very interested to see what complications you encountered with @samouss with conditional types (for my own curiosity 😉 )
We've tried with both union types and conditional types. With the latter we're almost there but not yet. Once we put the widgets inside an array we're not able to find which kind of widget it is. It means that we only have access to all the common attributes of each kind of widget (which makes perfect sense). Here is an example. Note that I might be completely wrong about the subject though.
my fear is that the user get the same issue when calling
widget.init
This is not an issue because we call the methods for the users. They never have to call it directly.
Another solution would be to enforce init, render and dispose from now on. WDYT?
Yes that would solve the issue but I would avoid to do it right now. I would rather suggest to take this into consideration once we revamp the core of InstantSearch. Until now we can treat them as optional inside the codebase it was (almost) already the case.
b3ce674
to
1404d2f
Compare
I updated this PR so that it reflects only changes in the source files. |
@@ -59,7 +64,11 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/query-rule- | |||
container: document.createElement('div'), | |||
}); | |||
|
|||
widget.init({ helper, state: helper.state, instantSearchInstance: {} }); | |||
widget.init!({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would be very interested to see what complications you encountered with @samouss with conditional types (for my own curiosity 😉 )
We've tried with both union types and conditional types. With the latter we're almost there but not yet. Once we put the widgets inside an array we're not able to find which kind of widget it is. It means that we only have access to all the common attributes of each kind of widget (which makes perfect sense). Here is an example. Note that I might be completely wrong about the subject though.
my fear is that the user get the same issue when calling
widget.init
This is not an issue because we call the methods for the users. They never have to call it directly.
Another solution would be to enforce init, render and dispose from now on. WDYT?
Yes that would solve the issue but I would avoid to do it right now. I would rather suggest to take this into consideration once we revamp the core of InstantSearch. Until now we can treat them as optional inside the codebase it was (almost) already the case.
1404d2f
to
591b3de
Compare
591b3de
to
c82c4f8
Compare
c82c4f8
to
b7b7869
Compare
# [3.3.0](v3.2.1...v3.3.0) (2019-04-11) ### Bug Fixes * **connectQueryRules:** improve tracked refinement type ([#3648](#3648)) ([e16ad57](e16ad57)) * **currentRefinements:** don't rely on ([#3672](#3672)) ([cd64bcf](cd64bcf)) * **queryRuleCustomData:** add default template ([#3650](#3650)) ([83e9eaa](83e9eaa)) * **QueryRuleCustomData:** pass data as object to templates ([#3647](#3647)) ([b8f8b4e](b8f8b4e)) * **queryRules:** fix types and stories ([#3670](#3670)) ([ba6e2e6](ba6e2e6)) * **routing:** apply windowTitle on first load ([#3669](#3669)) ([d553502](d553502)), closes [#3667](#3667) * **routing:** support parsing URLs with up to 100 refinements ([#3671](#3671)) ([6ddcfb6](6ddcfb6)) * **RoutingManager:** avoid stale uiState ([#3630](#3630)) ([e1588aa](e1588aa)) * **types:** improve InstantSearch types ([#3651](#3651)) ([db9b91e](db9b91e)) * **ua:** Update the User-Agent to use the new format ([#3616](#3616)) ([ab84c57](ab84c57)) ### Features * **infiniteHits:** add previous button ([#3645](#3645)) ([2c9e38d](2c9e38d)) * **queryRules:** add connectQueryRules connector ([#3597](#3597)) ([924cd99](924cd99)), closes [#3599](#3599) [#3600](#3600) * **queryRules:** add context features to Query Rules ([#3617](#3617)) ([922879e](922879e)), closes [#3602](#3602) ### Reverts * feat(infiniteHits): add previous button ([214c0fc](214c0fc))
This completes the InstantSearch, Helper, Client and Widget types, and updates the code impacted.