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(client): support node #96

Merged
merged 2 commits into from
Nov 12, 2021
Merged

feat(client): support node #96

merged 2 commits into from
Nov 12, 2021

Conversation

wangsijie
Copy link
Contributor

Summary

Do 2 things to support running both in node and browser(already supported):

  1. can pass in custom fetchFunction in constructer
  2. onRedirect parameter, defaults to window.location.assign

Testing

change testEnvironment to node and run test for file index.test.ts

Copy link
Contributor

@simeng-li simeng-li left a comment

Choose a reason for hiding this comment

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

overall LGTM. one thing cross my mind, since requester is widely used cross the SDK. should we use a webpack.ProvidePlugin to alias the logto requester. so no need to import in every file.

Also just wondering, if developer provide their own requester, then for the error thrown logics, would it be in-consistent?

@wangsijie
Copy link
Contributor Author

overall LGTM. one thing cross my mind, since requester is widely used cross the SDK. should we use a webpack.ProvidePlugin to alias the logto requester. so no need to import in every file.

Also just wondering, if developer provide their own requester, then for the error thrown logics, would it be in-consistent?

LOG-262

@wangsijie wangsijie force-pushed the wangsijie-client-node branch from e463b28 to 6e4d82b Compare November 12, 2021 06:30
@wangsijie wangsijie requested a review from gao-sun November 12, 2021 06:30
@wangsijie wangsijie merged commit 367ec03 into master Nov 12, 2021
@wangsijie wangsijie deleted the wangsijie-client-node branch November 12, 2021 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants