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

Feature/rebase search bar #1515

Closed
wants to merge 32 commits into from
Closed

Conversation

tmeyer2115
Copy link
Collaborator

No description provided.

tmeyer2115 and others added 26 commits April 21, 2021 07:47
This PR adds a new entry point to the SDK's source. This entry point defines
a class, `AnswersSearchBar`, that powers a `SearchBar` only integration. The
class is defined in `answers-search-bar.js`, which bears many similarities to
`answers-umd.js`. There are a few key differences:
- The `FilterNodeFactory`, `formatRichText`, and `processTranslations` methods
are not exported on the top-level object. These are not needed for this type of
integration.
- We will not handle CSS variables in this integration. Styling is very limited
and prescriptive. So, all related ponyfills were removed.
- It is assumed that the client has pulled down the `TemplateBundle` on their
own and we will not need to load it for them. So, the `DefaultTemplateLoader`
is not used.
There may be room for further improvement. I left many of the storage
interactions that are present in `answers-umd.js`, but some of these are likely
irrelevant for a stand-alone `SearchBar`. I will do a closer inspection in a
later PR.

A new registry of `Components`, specific to this type of integration, was
added. This registry was put in a new file, `search-bar-only-registry.js`. At
first, I attempted to export a second registry from the existing `registry.js`.
But, Gulp's tree-shaking was still including all `Component` classes in the
`SearchBar`-only bundle. Creating a new file solved that issue.

Finally, the `ComponentManager` class is no longer a singleton. This is because
it can now accept different `componentRegistry`s. I don't think this should
cause an issue. The `ANSWERS` and `ANSWERS_SEARCH_BAR` instances are already
singletons. So, if you attempt to `init` either twice on your page, you would
already get the same `componentManager`. Likewise if you tried to make multiple
instances of either on your page.

J=SLAP-1271
TEST=manual

Built a new JS and TemplateBundle asset from this entry point. Verified the
following:

- The `SearchBar` appears on the page and works as expected.
- The `Autocomplete` works as expected.
- The `setContext` method could add a context to the search.
- The `redirectUrl` worked as expected and the correct query params were added
  to it.
- The `defaultInitialSearch` worked as expected.
- I could configure the `SearchBar` component.
The `setSessionsOptIn` method worked as expected.
This PR further optimizes the stand-alone `SearchBar` asset by removing unused
code from its entry point. Specifically, the following was removed:
- Any code related to `VerticalPagesConfig` as there is no navigation or
  no results in this integration.
- The `updateListener` and `resetListener` passed to `Storage`. This
  integration never persists any data in storage. Because of this, if there
  were a pop history event, any URL params would be unrelated to the
  `SearchBar`. This allows us to safely remove the listeners as they will
  never be invoked in this type of integration.
- The `QUERY_ID` storage listener was also removed. This integration doesn't
  actually perform a search, so there will never be a `QUERY_ID`. By the same
  reasoning, references to the `QueryUpdateListener` could also be removed.
- The `scrollListener` was also removed as that is not desired on a
  `SearchBar`-only page.
The other top-level methods, such as `setContext` and `setConversionsOptIn`
were left in, as requested by Product.

J=SLAP-1271
TEST=manual

Ensured the basic functionality of the integration still worked (autocomplete,
clear button, and redirects).
This PR adds the build chains for the modern JS bundle and UMD template bundle
of the `SearchBar` integration. Support for legacy JS bundles and an IIFE
template bundle will be added in the followup epic. Having worked with our
build chain architecture, I think it needs to be cleaned up. The hierarchy of
classes and factories is difficult to follow. I will make a followup item for
this.

J=SLAP-1272
TEST=manual

Ran the acceptance tests to make sure the existing, full SDK assets still were
built properly. Used the `SearchBar`-only assets only a simple site to ensure
they could power that type of integration properly.
This PR adds a new CSS asset, `answers-search-bar.css`, that contains just the
styling needed for the `SearchBar` integration. Specifically, this bundle
contains some common SDK styling and styles for the `SearchBar`, `AutoComplete`,
and `Icon` components.

J=SLAP-1254
TEST=manual

Built a `SearchBar`-only page locally with this new CSS asset. Saw the correct
styling applied and verified that the page score on Mobile was still 100.
This PR adds IIFE and UMD bundles of the `SearchBar` that support IE11. As part
of this, I had to add CSS Variable Ponyfills back to the integration's entry
point. Technically, we could create styling for this integration that does not
use CSS variables. But, that would be rather onerous to create and maintain.

J=SLAP-1253
TEST=manual

Created a `SearchBar` site using the IIFE and UMD assets. Verified that the site
looked and worked as expected in IE11.
This PR renames the `SearchBar`-only integration's exported object from `ANSWERS_SEARCH_BAR` to `ANSWERS`. This was a request made by Product.

TEST=manual

Verified the name change on a local site.
This PR ensures that an IIFE template bundle is generated for this integration.

TEST=manual

Used the new asset on an IE11 site and verified the integration worked as
expected. Verified that the file itself was in IIFE-style.
The tests verify a simple flow: a user interacts with the search bar, then selects an autocomplete option, and is redirected to the proper URL.

J=SLAP-1364
TEST=auto

Ensured the new tests passed.
npm-force-resolutions ports a yarn only feature where
you can pin transient dependencies to a specific version.

We're using it for regenerator-runtime so that a searchbar
only integration will comply with a CSP prohibiting unsafe-eval.

Due to testcafe not working with <meta> tags prescribing CSP,
we will need some workaround to get an acceptance test for this.

This commit will be cherrypicked into support/v1.8

J=SLAP-1433
TEST=manual

test that I can load the searchbar with a meta tag prohibiting unsafe-eval
see that trying to use v1.9 without these changes causes the searchbar to
not render at all
This commit bumps the answers-core version, so that all GET
requests use credentials: 'include', which allow cookies to be
sent across origins.

This fixes an issue with the session-id cookie not being sent,
and therefore a new session-id being generated by the backend after
every search.

Will cherry pick this (and then test) on support/v1.8 and develop

T=TECHOPS-1384
TEST=manual

see that the session-id cookie is sent in search requests

Co-authored-by: Oliver Shi <oshi@yext.com>
This PR adds a new entry point to the SDK's source. This entry point defines
a class, `AnswersSearchBar`, that powers a `SearchBar` only integration. The
class is defined in `answers-search-bar.js`, which bears many similarities to
`answers-umd.js`. There are a few key differences:
- The `FilterNodeFactory`, `formatRichText`, and `processTranslations` methods
are not exported on the top-level object. These are not needed for this type of
integration.
- We will not handle CSS variables in this integration. Styling is very limited
and prescriptive. So, all related ponyfills were removed.
- It is assumed that the client has pulled down the `TemplateBundle` on their
own and we will not need to load it for them. So, the `DefaultTemplateLoader`
is not used.
There may be room for further improvement. I left many of the storage
interactions that are present in `answers-umd.js`, but some of these are likely
irrelevant for a stand-alone `SearchBar`. I will do a closer inspection in a
later PR.

A new registry of `Components`, specific to this type of integration, was
added. This registry was put in a new file, `search-bar-only-registry.js`. At
first, I attempted to export a second registry from the existing `registry.js`.
But, Gulp's tree-shaking was still including all `Component` classes in the
`SearchBar`-only bundle. Creating a new file solved that issue.

Finally, the `ComponentManager` class is no longer a singleton. This is because
it can now accept different `componentRegistry`s. I don't think this should
cause an issue. The `ANSWERS` and `ANSWERS_SEARCH_BAR` instances are already
singletons. So, if you attempt to `init` either twice on your page, you would
already get the same `componentManager`. Likewise if you tried to make multiple
instances of either on your page.

J=SLAP-1271
TEST=manual

Built a new JS and TemplateBundle asset from this entry point. Verified the
following:

- The `SearchBar` appears on the page and works as expected.
- The `Autocomplete` works as expected.
- The `setContext` method could add a context to the search.
- The `redirectUrl` worked as expected and the correct query params were added
  to it.
- The `defaultInitialSearch` worked as expected.
- I could configure the `SearchBar` component.
The `setSessionsOptIn` method worked as expected.
This PR further optimizes the stand-alone `SearchBar` asset by removing unused
code from its entry point. Specifically, the following was removed:
- Any code related to `VerticalPagesConfig` as there is no navigation or
  no results in this integration.
- The `updateListener` and `resetListener` passed to `Storage`. This
  integration never persists any data in storage. Because of this, if there
  were a pop history event, any URL params would be unrelated to the
  `SearchBar`. This allows us to safely remove the listeners as they will
  never be invoked in this type of integration.
- The `QUERY_ID` storage listener was also removed. This integration doesn't
  actually perform a search, so there will never be a `QUERY_ID`. By the same
  reasoning, references to the `QueryUpdateListener` could also be removed.
- The `scrollListener` was also removed as that is not desired on a
  `SearchBar`-only page.
The other top-level methods, such as `setContext` and `setConversionsOptIn`
were left in, as requested by Product.

J=SLAP-1271
TEST=manual

Ensured the basic functionality of the integration still worked (autocomplete,
clear button, and redirects).
This PR adds the build chains for the modern JS bundle and UMD template bundle
of the `SearchBar` integration. Support for legacy JS bundles and an IIFE
template bundle will be added in the followup epic. Having worked with our
build chain architecture, I think it needs to be cleaned up. The hierarchy of
classes and factories is difficult to follow. I will make a followup item for
this.

J=SLAP-1272
TEST=manual

Ran the acceptance tests to make sure the existing, full SDK assets still were
built properly. Used the `SearchBar`-only assets only a simple site to ensure
they could power that type of integration properly.
This PR adds a new CSS asset, `answers-search-bar.css`, that contains just the
styling needed for the `SearchBar` integration. Specifically, this bundle
contains some common SDK styling and styles for the `SearchBar`, `AutoComplete`,
and `Icon` components.

J=SLAP-1254
TEST=manual

Built a `SearchBar`-only page locally with this new CSS asset. Saw the correct
styling applied and verified that the page score on Mobile was still 100.
This PR adds IIFE and UMD bundles of the `SearchBar` that support IE11. As part
of this, I had to add CSS Variable Ponyfills back to the integration's entry
point. Technically, we could create styling for this integration that does not
use CSS variables. But, that would be rather onerous to create and maintain.

J=SLAP-1253
TEST=manual

Created a `SearchBar` site using the IIFE and UMD assets. Verified that the site
looked and worked as expected in IE11.
This PR renames the `SearchBar`-only integration's exported object from `ANSWERS_SEARCH_BAR` to `ANSWERS`. This was a request made by Product.

TEST=manual

Verified the name change on a local site.
This PR ensures that an IIFE template bundle is generated for this integration.

TEST=manual

Used the new asset on an IE11 site and verified the integration worked as
expected. Verified that the file itself was in IIFE-style.
The tests verify a simple flow: a user interacts with the search bar, then selects an autocomplete option, and is redirected to the proper URL.

J=SLAP-1364
TEST=auto

Ensured the new tests passed.
npm-force-resolutions ports a yarn only feature where
you can pin transient dependencies to a specific version.

We're using it for regenerator-runtime so that a searchbar
only integration will comply with a CSP prohibiting unsafe-eval.

Due to testcafe not working with <meta> tags prescribing CSP,
we will need some workaround to get an acceptance test for this.

This commit will be cherrypicked into support/v1.8

J=SLAP-1433
TEST=manual

test that I can load the searchbar with a meta tag prohibiting unsafe-eval
see that trying to use v1.9 without these changes causes the searchbar to
not render at all
This commit bumps the answers-core version, so that all GET
requests use credentials: 'include', which allow cookies to be
sent across origins.

This fixes an issue with the session-id cookie not being sent,
and therefore a new session-id being generated by the backend after
every search.

Will cherry pick this (and then test) on support/v1.8 and develop

T=TECHOPS-1384
TEST=manual

see that the session-id cookie is sent in search requests

Co-authored-by: Oliver Shi <oshi@yext.com>
@tmeyer2115 tmeyer2115 added the WIP Work in progress label Aug 17, 2021
@coveralls
Copy link

coveralls commented Aug 17, 2021

Coverage Status

Coverage decreased (-1.6%) to 58.851% when pulling 70545a2 on feature/rebase-search-bar into e7eb7a9 on develop.

@tmeyer2115 tmeyer2115 closed this Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants