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

gracefully handle analytics error #1774

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Aug 31, 2022

Throw error from not having ytag script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added ANSWERS.setConversionsOptIn(true); to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

Screen Shot 2022-08-31 at 8 36 25 AM

Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any idea why code coverage is failing? besides that, lgtm!

@nmanu1
Copy link
Contributor

nmanu1 commented Aug 31, 2022

also, I was just curious, so I looked it up and generate-license-file released a new major version a couple days ago, which is why the third party notices changed. we don't pin to any major when installing the package in the GH action

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 31, 2022

any idea why code coverage is failing? besides that, lgtm!

the default for comparision_branch for coverage workflow is "main" which we have "master" instead for SDK. updated in this pr to point to master

Copy link
Contributor

@nmanu1 nmanu1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm once acceptance tests pass!

@yen-tt
Copy link
Contributor Author

yen-tt commented Aug 31, 2022

also, I was just curious, so I looked it up and generate-license-file released a new major version a couple days ago, which is why the third party notices changed. we don't pin to any major when installing the package in the GH action

A quick look at the release notes for v2 and the changes seem ok? We can make an item or a separate pr to pin to v1 in the reusuable workflow repo or pin to v2 and probably have to update the version package.json to v2 in our repos as well so it's consistent

@nmanu1
Copy link
Contributor

nmanu1 commented Aug 31, 2022

A quick look at the release notes for v2 and the changes seem ok? We can make an item or a separate pr to pin to v2 in the reusuable workflow repo.

Yeah, that's what I was thinking. There weren't any breaking changes on the running side, only on the output produced and those changes seem fine. But, that might not be the case if they release a v3, so it would be good to pin to v2 in another item

@yen-tt yen-tt merged commit bb27272 into hotfix/v1.14.2 Aug 31, 2022
@yen-tt yen-tt deleted the dev/handle-analytics-error branch August 31, 2022 13:54
@yen-tt yen-tt mentioned this pull request Aug 31, 2022
yen-tt added a commit that referenced this pull request Aug 31, 2022
### Bug Fixes
- Gracefully handle analytics-related error to prevent uncaught errors from blocking the search on load (#1774)
@tmeyer2115 tmeyer2115 mentioned this pull request Sep 2, 2022
tmeyer2115 added a commit that referenced this pull request Sep 2, 2022
### Bug Fixes
- Gracefully handle analytics-related error to prevent uncaught errors from blocking the search on load. (#1774)
- Pinned the Google Maps API version to v3.47. This is the last one to officially support IE11. (#1681)
@yen-tt yen-tt mentioned this pull request Sep 2, 2022
yen-tt added a commit that referenced this pull request Sep 2, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1786" alt="Screen Shot 2022-08-31 at 8 36 25 AM" src="https://user-images.githubusercontent.com/36055303/187679764-b5c9a6e6-bf12-4467-a42b-66fb3052846d.png">
yen-tt added a commit that referenced this pull request Sep 2, 2022
Throw error from not having `ytag` script tag for conversion tracking opt in would prevent a search fired on load. This is an aggressive behavior as analytics shouldn't affect search behavior. This pr updated the two throw error into console error instead.

Same as #1771 but without acceptance tests changes

TEST=manual

Added `ANSWERS.setConversionsOptIn(true);` to a local index page with Answers setup. see that the previously uncaught throw error is now a console error and a search on load is no longer blocked.

<img width="1786" alt="Screen Shot 2022-08-31 at 8 36 25 AM" src="https://user-images.githubusercontent.com/36055303/187679764-b5c9a6e6-bf12-4467-a42b-66fb3052846d.png">
yen-tt added a commit that referenced this pull request Sep 2, 2022
### Bug Fixes
- Gracefully handle analytics-related error to prevent uncaught errors from blocking the search on load (#1774)
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.

None yet

3 participants