Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

fix(highlight): wrong parsing between client and server #183

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

mthuret
Copy link
Contributor

@mthuret mthuret commented Jul 24, 2017

See #168

The Server-side html is rendered perfectly but then when rendered on the client using the resultsState props, results are containing the server old timestamp and <Highlight> the new client one. The <Highlight/> component then doesn't see any highlighted values and that's why we can see the little glitch.

For now, the solution is to remove the timestamp when dealing with highlighting tags.

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.

Although @vvo said that the timestamp is against an attack vector, I think it's fine not to have it here.

@algobot
Copy link
Contributor

algobot commented Jul 24, 2017

Deploy preview ready!

Built with commit d190ba6

https://deploy-preview-183--react-instantsearch.netlify.com

@Haroenv
Copy link
Contributor

Haroenv commented Jul 24, 2017

I think this is a breaking change though, you could do s/<ais-highlight-\d{6}/>/<em>/g, and this would break it, no?

@mthuret
Copy link
Contributor Author

mthuret commented Jul 24, 2017

@Haroenv => for people using regex and setDangerousHtml (for parsing arrays for instance), yes. Will document that in the migration guide when releasing. Hopefully this shouldn't be done by too many people.

@Haroenv
Copy link
Contributor

Haroenv commented Jul 24, 2017

In Yarn I imported the highlightTags module, so it should be safe to upgrade, I think this methods is the most used, so LGTM, other info was internal, so I think it's safe

BREAKING CHANGE:

We remove the timestamp present in our highlight preTag and postTag. If you were using regex to parse the
highlighting results then you'll need to adapt it as now it's only "ais-highlight".
@mthuret mthuret force-pushed the fix/ssr-highlighting branch from d4ba75f to d190ba6 Compare July 24, 2017 13:52
@mthuret
Copy link
Contributor Author

mthuret commented Jul 24, 2017

@Haroenv Is it because you need arrays support? We should be careful if we move the file then.

@Haroenv
Copy link
Contributor

Haroenv commented Jul 24, 2017

yes, for arrays I import highlightTags from 'react-instantsearch/src/core/highlightTags';, which will still work after this change

@vvo
Copy link
Contributor

vvo commented Jul 25, 2017

Also if people were importing and using stuff that is not documented this is not a breaking change I believe. This was not public API.

@mthuret mthuret merged commit 2daae70 into master Jul 25, 2017
@mthuret mthuret deleted the fix/ssr-highlighting branch July 25, 2017 09:43
mthuret pushed a commit that referenced this pull request Jul 25, 2017
<a name="4.1.0-beta.3"></a>
# [4.1.0-beta.3](v4.1.0-beta.2...v4.1.0-beta.3) (2017-07-25)

### Bug Fixes

* **error:** reset error when receiving results of a query (not when sending it) (#179) ([bb12c29](bb12c29))
* **highlight:** wrong parsing between client and server (#183) ([2daae70](2daae70))
* **poweredBy:** SSR compatibility (#181) ([ec0fa8a](ec0fa8a))

### BREAKING CHANGES

* **highlight:** We remove the timestamp present in our highlight preTag and postTag. If you were using regex to parse the
highlighting results then you'll need to adapt it as now it's only "ais-highlight".
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants