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

Improved error message logging in console #573

Merged
merged 7 commits into from
Aug 11, 2022

Conversation

arielsilvestri
Copy link
Contributor

@arielsilvestri arielsilvestri commented Aug 9, 2022

Adds a console error with the error status text from the API explaining to users what went wrong with the settings call.

Screen Shot 2022-08-03 at 12 57 19 PM

.

@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2022

🦋 Changeset detected

Latest commit: 9fb8918

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@segment/analytics-next Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -26,6 +26,8 @@ const cdnResponse: LegacySettings = {

const fetchSettings = Promise.resolve({
json: () => Promise.resolve(cdnResponse),
text: () => Promise.resolve('text'),
Copy link
Contributor

@silesky silesky Aug 9, 2022

Choose a reason for hiding this comment

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

Ugh: these naive http mocks are brittle -- I've been using https://github.com/segmentio/analytics-next/blob/create-rc-of-analytics-node/packages/browser/src/test-helpers/factories.ts in new tests and have refactored some of the old tests to use them, but they're still all over.

@@ -83,9 +83,16 @@ export function loadLegacySettings(
const baseUrl = cdnURL ?? getCDN()

return fetch(`${baseUrl}/v1/projects/${writeKey}/settings`)
.then((res) => res.json())
.then((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! Can we get some client tests?

@silesky
Copy link
Contributor

silesky commented Aug 9, 2022

We may need to update all the tests that use mock fetch to use more robust HTTP mock, since it's checking for OK
see my comment: #573 (comment)
image

Another reason why using nock / msw is in general a recommended approach over mocking fetch.

@arielsilvestri arielsilvestri merged commit 6203c20 into master Aug 11, 2022
@arielsilvestri arielsilvestri deleted the error-message-console-logging branch August 11, 2022 20:08
@github-actions github-actions bot mentioned this pull request Aug 11, 2022
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.

3 participants