-
Notifications
You must be signed in to change notification settings - Fork 1
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
Make client isomorphic as a node library #49
Conversation
This allows the client library to be used in node applications, with the small change that `env` will be required, and the application will need to pass in an isomorphic replacement for `fetch` (presumably `node-fetch`).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I'm going to add @dougmartin as a co-reviewer though just to double check what he thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required but I made a suggestion for adding an exception if fetch
is not resolved.
const {jwt, serviceUrl} = options; | ||
this.jwt = jwt; | ||
this.env = options.env || getEnv(); | ||
this.serviceUrl = getServiceUrlFromQueryString() || serviceUrl || getServiceUrlFromEnv(this.env) || serviceUrls.production; | ||
this.fetch = ("fetch" in options) ? options.fetch : fetch; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to throw an exception here if this.fetch === undefined
. This will make debugging easier for future users. The exception could be something like:
new Error("fetch not found in options or window object")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sounds good, added.
} | ||
if (options?.body) { | ||
requestOptions.body = JSON.stringify(options.body); | ||
} | ||
return fetch(url, requestOptions) | ||
return this.fetch(url, requestOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note about throwing an exception in the constructor as this.fetch
could be undefined here if not found in the options (node) or in the window object (browser).
This allows the client library to be used in node applications, with the small change that
env
will be required, and the application will need to pass in an isomorphic replacement forfetch
(presumablynode-fetch
).