fix(Template): Harden Symbol checks in case of incorrect polyfill implementation

[bronzehedwick]

Summary

If the Google Maps API Symbol polyfill is present (which is included with the library), the instantsearch script presumes that Symbol is fully implemented. However, Symbol polyfills cannot replicate the behavior fully.

This causes the instantsearch script to error out and cause problems on
the page if a Symbol polyfill is used.

This commit hardens the Symbol checks to make sure real Symbols are
supported, not just a polyfill.

NOTE: The google maps API library is a common library that implements a
Symbol polyfill.

@see:

• https://stackoverflow.com/questions/48532850/is-google-maps-js-library-polyfilling-symbol
• https://stackoverflow.com/questions/39803281/get-typeof-of-the-value-in-es6#39805240

Result

[algobot]

Deploy preview for algolia-instantsearch ready!

Built with commit 92beae0

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

[bronzehedwick]

My commits don't pass the check, but it doesn't tell me how/why...

[Haroenv]

Commits didn't pass because it wasn't following the commit convention, I edited them, sorry for the delay, we were having our offsite and looked at PRs with a lower frequency. I think this PR makes sense, but maybe we could even get rid of this function completely in next major (since the react elements as template have been disabled for a few months already)

[Haroenv]

Do you confirm that this actually fixes the issue though, your screenshot highlights part of code in Preact, not in InstantSearch

[vvo]

@bronzehedwick This seems to be very similar to preactjs/preact-compat#423 (read the comment thread). Since you are also using IE11

[vvo]

Do you confirm that this actually fixes the issue though, your screenshot highlights part of code in Preact, not in InstantSearch

Yes would also like some more details on this.

[vvo]

Finally I think the issue IS really inside preact-compat since someone asked to strengthen the Symbol check more deeply: preactjs/preact-compat#440

If so we can push for a new release of preact-compat by answering the question from preact author in the PR.

On our side we might want to completely remove support for React elements in V2 or in V3.. and delete the code. This is a non-documented feature.

For now we could move the const REACT_ELEMENT.. to an inner function so it only runs when someone tries to use React elements as templates.

[iam4x]

I agree with @Haroenv we might want to remove this completely. We deprecated React components as templates almost a year ago, it has been done to ease the v1 -> v2 transition.

What do you think @bobylito ?

[bobylito]

I agree with @Haroenv we might want to remove this completely. We deprecated React components as templates almost a year ago, it has been done to ease the v1 -> v2 transition.

We actually removed the support for it. https://community.algolia.com/instantsearch.js/v2/guides/migration.html#react-components-cant-be-used-as-templates However this function is for checking that a user who just migrated is not using them by mistake. I'd say we should keep this message until v3, and therefore keep the check.

For now we could move the const REACT_ELEMENT.. to an inner function so it only runs when someone tries to use React elements as templates.

I don't think this could work because it will be used to check if this is a react element. Even if it's not a react element.

[bronzehedwick]

@Haroenv @vvo Yes, I did confirm this fixed the issue in IE 11. What seemed to be the issue is that Google Maps API defines a Symbol polyfill in the global namespace.

I can't make a jsfiddle or some such because I can't expose our Google API keys, but you can confirm this by going to https://mskcc.org/locations/directory in IE, and doing typeof Symbol in the console, then trying the same at https://mskcc.org, where we don't load the Google Maps lib. The former should be function, while the latter should be undefined.

Since Google Maps polyfills Symbol, but the polyfill isn't complete, the check for Symbol comes back "yes" on IE, then, I assume, preact tries to use Symbol methods that the polyfill doesn't support.

The typeof Symbol.iterator === 'symbol' check just makes sure that we're dealing with a real Symbol, since that feature can't be polyfilled.

Hope that provides enough information.

[bronzehedwick]

To recap, there seems to be 2 solutions to this problem:

1. The fix I've committed
2. Removing preact

If the second solution is preferred, what would be the timeline for that? My hunch is that it would be longer than the first.

I ask because this bug delayed our Algolia launch, and I'm trying to assess whether we should release with a fork of the code for the time being or wait for a fix from the Algolia team.

Thanks!

[bobylito]

If the second solution is preferred, what would be the timeline for that? My hunch is that it would be longer than the first.

There is a misunderstanding here. The code you've fixed is not related to Preact directly, it is a way for us to detect react elements used as templates.

Your solution seems fine to me. Eventually we'll move for something that doesn't rely on detection at all :)

[bobylito]

Thanks @bronzehedwick

[bronzehedwick]

Great, thanks all!