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(bindEvent): escape payload correctly #4670

Merged
merged 6 commits into from
Mar 9, 2021
Merged

fix(bindEvent): escape payload correctly #4670

merged 6 commits into from
Mar 9, 2021

Conversation

eunjae-lee
Copy link
Contributor

@eunjae-lee eunjae-lee commented Mar 5, 2021

Summary

This PR updates the way we serialize & deserialize payloads for bindEvent. It's internal and users never got to deserialize by themselves, so it's not a breaking change.

Details

Since "add hits and attributes to InsightsEvent" PR, bindEvent method now serializes hits as well. And hits can contain special characters unlike the payloads that we used to have in insights middleware.

For example,

btoa('™')
> Uncaught DOMException: String contains an invalid character

but

btoa(encodeURIComponent('™'))
> "JUUyJTg0JUEy"

decodeURIComponent(atob("JUUyJTg0JUEy"))
> "™"

A minor concern

It's actually not about this PR, but about the previous "add hits and attributes to InsightsEvent" PR.
Now that we expose hits to InsightsEvent, its payload is much bigger than before. It's going to bloat the DOM output.
When they have templates like

<button
  type="button"
  ${bindEvent('click', hit, 'Favorite')}
>
 ...
</button>

If they don't pass the whole hit object, and pass only a subset of it, it will reduce the size. What do you think?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 5, 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.

Latest deployment of this branch, based on commit 3f29d79:

Sandbox Source
InstantSearch.js Configuration

@eunjae-lee eunjae-lee changed the title fix(bindEvent): qoute the value fix(bindEvent): WIP Mar 5, 2021
@eunjae-lee eunjae-lee changed the title fix(bindEvent): WIP fix(bindEvent): escape payload correctly Mar 8, 2021
@eunjae-lee eunjae-lee marked this pull request as ready for review March 8, 2021 13:38
@eunjae-lee eunjae-lee requested review from a team, yannickcr and Haroenv and removed request for a team March 8, 2021 13:38
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.

reasonable fix!

@eunjae-lee
Copy link
Contributor Author

reasonable fix!

You also think the A minor concern part is okay?

@Haroenv
Copy link
Contributor

Haroenv commented Mar 8, 2021

it's probably not really an issue IMO, although a subset was already set with objectIDs, which you said was insufficient for segment

@eunjae-lee
Copy link
Contributor Author

it's probably not really an issue IMO, although a subset was already set with objectIDs, which you said was insufficient for segment

What I meant by a subset was to pass an object made from hit with some properties omitted like, for example, image_url, description, ... that won't be used in the insights middleware.

It could bloat the html size and might be an issue in case of SSR. Even if so, they can omit some properties to avoid the issue.

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.

2 participants