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

enhancement: error handling + exp flag #31

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Conversation

AmeanAsad
Copy link
Contributor

Changes:

  • Add experimental flag to load nodes.
  • Ignores fallback on certain errors and status codes.
  • Test cases for those fallback edge cases

@AmeanAsad AmeanAsad requested a review from guanzo October 27, 2023 21:25
@@ -54,7 +56,7 @@ export class Saturn {
this._monitorPerformanceBuffer()
}
this.storage = this.opts.storage || memoryStorage()
this.loadNodesPromise = this._loadNodes(this.opts)
this.loadNodesPromise = this.opts.experimental ? this._loadNodes(this.opts) : null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DiegoRBaquero added this to only load nodes for the % we desire per a / b testing.

src/client.js Outdated
@@ -81,7 +83,7 @@ export class Saturn {
}
}

const origins = options.origins
const origins = options.origins || [options.cdnURL]
Copy link
Collaborator

@guanzo guanzo Oct 27, 2023

Choose a reason for hiding this comment

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

if options.origins is an empty array it's still truthy so it won't fallback to cdnURL, is that ok here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not ok, thanks for flagging. I've added more logic to handle this better.

src/client.js Outdated
@@ -81,7 +83,7 @@ export class Saturn {
}
}

const origins = options.origins
const origins = options.origins || [options.cdnURL]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const origins = options.origins || [options.cdnURL]
const origins = options.origins ?? [options.cdnURL]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DiegoRBaquero updated this with your suggestion and eric's comment above

src/client.js Outdated Show resolved Hide resolved
@AmeanAsad AmeanAsad merged commit 8b532ec into main Oct 27, 2023
1 check passed
@AmeanAsad AmeanAsad deleted the feat/error-handling branch October 27, 2023 22:24
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