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

feat: add customer origin url fallback #37

Merged
merged 14 commits into from
Nov 3, 2023
Merged

Conversation

AmeanAsad
Copy link
Contributor

Changes:

  • Simplified options passing to only use a nodes array instead having (nodes, url, node) as accepted options.
  • Added a fallback to an originURL if it exists to fetch data as a final fallback.

src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
@guanzo
Copy link
Collaborator

guanzo commented Nov 1, 2023

Is there a way to document the fetch options in a DRY way similar to typescript, but with our current tooling?

const fetchOpts: object = {
   cdnURL: string,
   // etc
}

The amount of potential options has grown and I feel the jsdoc method of listing individual fields is getting a bit hefty, especially since there's multiple methods that accept the same options object.

EDIT: I guess @typedef is the way. https://jsdoc.app/tags-typedef.html

src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated
const jwt = await getJWT(this.opts, this.storage)
options.jwt = jwt
}
options.format = opts.format ? opts.format : 'car'
Copy link
Collaborator

@guanzo guanzo Nov 2, 2023

Choose a reason for hiding this comment

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

This will still return 'car' when we pass { format: null }. Now i'm wondering if it's better to create a client side only 'flat' format to avoid falsy checks defaulting to 'car', when in fact we want flat files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed this.

I think adding explicit file formats is better to identify flat or car. We only currently support car or flat though so I think the condition here is sufficient and we can change this later if needed if we start supporting more file formats.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll eventually need to support raw format too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to handle the file types explicitly in this PR or shall we do that later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets do another PR, this PR is too big already

src/client.js Outdated Show resolved Hide resolved
test/test-utils.js Outdated Show resolved Hide resolved
@AmeanAsad AmeanAsad merged commit 362c97e into main Nov 3, 2023
1 check passed
@AmeanAsad AmeanAsad deleted the feat/customer-fallback branch November 3, 2023 15:11
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