Skip to content

Conversation

ryanbas21
Copy link
Collaborator

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-3903 (has no details though)

Description

Took some liberties here, feel free to critique if this was not the direction we were thinking. there's a lot in here though in terms of how I went about errors, and I used function overloading for typing the functions since they can have two valid signatures which is obviously a different take than usual.

Open to changes

Copy link

changeset-bot bot commented Apr 11, 2025

⚠️ No Changeset found

Latest commit: dfa38a3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@ryanbas21 ryanbas21 changed the base branch from main to effects-package April 11, 2025 22:04
Copy link

nx-cloud bot commented Apr 11, 2025

View your CI Pipeline Execution ↗ for commit 44f4b9e.

Command Status Duration Result
nx affected -t build typecheck lint test e2e-ci ✅ Succeeded 1m 39s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-04-14 22:47:04 UTC

ported over many features of the SDK, and did a best effort to maintain
API surface.
initial implementation of local,session, and token storage to port from
old SDK into effects style patterns
Copy link
Contributor

github-actions bot commented Apr 14, 2025

Deployed c98d503 to https://ForgeRock.github.io/ping-javascript-sdk/pr-229/c98d5036c5b68dcd5f4634bfe370d69cb0dfa117 branch gh-pages in ForgeRock/ping-javascript-sdk

@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.35%. Comparing base (e581333) to head (44f4b9e).

Additional details and impacted files
@@               Coverage Diff                @@
##           effects-package     #229   +/-   ##
================================================
  Coverage            47.35%   47.35%           
================================================
  Files                   29       29           
  Lines                 1472     1472           
  Branches               148      148           
================================================
  Hits                   697      697           
  Misses                 775      775           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tokenStore: TokenStoreObject,
): Promise<Tokens | TokensError>;
export async function getTokens(config: ConfigOptions): Promise<Tokens | TokensError>;
export async function getTokens(
Copy link
Collaborator Author

@ryanbas21 ryanbas21 Apr 15, 2025

Choose a reason for hiding this comment

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

So this function is a perfect example of a function coloring problem.

This is because of the optional tokenStore we allow, which basically turns everything here async, even though localStorage and sessionStorage have sync methods.

In the legacy sdk we just turn localStorage and sessionStorage methods to async they really don't need to be

If i'm being honest i'm dying to use effect for code like this because it solves this problem (and a million others).

I think effect would work within the effects (haha) package and probably would be used in utilities as well.

The only outlier here is that I think we would need to use effect within the *-client folders for the run* methods to actually run the effect.

effect (the package) also fits exactly into the architecture design of calling "effects" at the edge (hence run* methods).

Note: I have not considered the use case of this code outside of using the client, so we would probably need to have a wrapper around this code in this concept, if effects itself was to be installed by a consumer and used directly.

@ryanbas21 ryanbas21 closed this Apr 17, 2025
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.

2 participants