Skip to content

feat: add-effects-package #224

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .changeset/config.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
],
"commit": false,
"fixed": [["@forgerock/*"]],
"privatePackages": false,
"linked": [],
"access": "public",
"baseBranch": "main",
Expand Down
8 changes: 8 additions & 0 deletions .changeset/empty-brooms-sneeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@forgerock/davinci-client': minor
'@forgerock/sdk-utilities': minor
'@forgerock/shared-types': minor
'@forgerock/effects': minor
---

added effects, utilities, and shared-types. ported over types and configuration settings from legacy sdk
6 changes: 6 additions & 0 deletions .changeset/lemon-rocks-dream.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@forgerock/davinci-client': minor
'@forgerock/effects': minor
---

add effects package
5 changes: 5 additions & 0 deletions .changeset/slick-cougars-smoke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@forgerock/sdk-utilities': minor
---

created sdk-utilities package
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ packages/**/coverage/
node_modules/
tests/**/app/index.js*
tests/**/ie11/ie-bundle.js*
**/out-tsc/*
.swc
.vite

Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# ForgeRock Javascript SDK Changelogs

[ForgeRock DaVinci Client package](./packages/davinci-client/CHANGELOG.md)
[ForgeRock SDK Utilities package](./packages/sdk-utilities/CHANGELOG.md)
7 changes: 4 additions & 3 deletions e2e/davinci-app/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import './style.css';

import { Config, FRUser, TokenManager } from '@forgerock/javascript-sdk';
import { davinci } from '@forgerock/davinci-client';

import type { DaVinciConfig, RequestMiddleware } from '@forgerock/davinci-client/types';

import textComponent from './components/text.js';
Expand All @@ -27,7 +26,7 @@ const searchParams = new URLSearchParams(qs);
const config: DaVinciConfig =
serverConfigs[searchParams.get('clientId') || '724ec718-c41c-4d51-98b0-84a583f450f9'];

const requestMiddleware: RequestMiddleware[] = [
const requestMiddleware: RequestMiddleware<'DAVINCI_NEXT' | 'DAVINCI_START'>[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use ActionTypes here?
RequestMiddleware<ActionTypes>

(fetchArgs, action, next) => {
if (action.type === 'DAVINCI_START') {
fetchArgs.url.searchParams.set('start', 'true');
Expand Down Expand Up @@ -55,7 +54,9 @@ const urlParams = new URLSearchParams(window.location.search);
if (continueToken) {
resumed = await davinciClient.resume({ continueToken });
} else {
await Config.setAsync(config);
// the current davinci-config has a slightly
// different middleware type than the old legacy config
await Config.setAsync(config as any);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the new middleware type does slightly differ from the original middleware type, I casted this temporarily.

Thats the only issue with this as it stands

}
function renderComplete() {
const clientInfo = davinciClient.getClient();
Expand Down
2 changes: 1 addition & 1 deletion e2e/davinci-app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"type": "module",
"private": true,
"nx": {
"tags": ["scope:app"]
"tags": ["scope:e2e"]
},
"scripts": {
"build": "pnpm nx nxBuild",
Expand Down
21 changes: 17 additions & 4 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export default [
{
files: ['**/*.ts', '**/*.tsx', '**/*.js', '**/*.jsx'],
rules: {
'import/extensions': [2, 'ignorePackages'],
'@nx/enforce-module-boundaries': [
'warn',
{
Expand All @@ -65,14 +66,26 @@ export default [
depConstraints: [
{
sourceTag: 'scope:e2e',
onlyDependOnLibsWithTags: ['scope:app'],
onlyDependOnLibsWithTags: ['scope:package'],
},
{
sourceTag: 'scope:package',
onlyDependOnLibsWithTags: [],
onlyDependOnLibsWithTags: ['scope:utilities', 'scope:effects', 'scope:shared-types'],
},
{
sourceTag: 'scope:utilities',
onlyDependOnLibsWithTags: ['scope:shared-types'],
},
{
sourceTag: 'scope:effects',
onlyDependOnLibsWithTags: ['scope:utilities', 'scope:shared-types'],
},
{
sourceTag: 'scope:config',
onlyDependOnLibsWithTags: ['scope:utilities', 'scope:shared-types'],
},
{
sourceTag: 'scope:types',
sourceTag: 'scope:shared-types',
onlyDependOnLibsWithTags: [],
},
],
Expand Down Expand Up @@ -133,7 +146,7 @@ export default [
},
},
{
ignores: ['dist/*', '**/**/tsconfig.spec.vitest-temp.json'],
ignores: ['**/*.md', 'dist/*', '**/**/tsconfig.spec.vitest-temp.json'],
},
{
...packageJson,
Expand Down
20 changes: 17 additions & 3 deletions nx.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
"!{projectRoot}/test-setup.[jt]s",
"noEslintConfig",
"noMarkdown",
"workspaceRootIgnores"
"workspaceRootIgnores",
"!{projectRoot}/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)"
],
"noMarkdown": ["!{projectRoot}/*.md", "!{projectRoot}/**/*.md"],
"noTests": [
Expand Down Expand Up @@ -69,7 +70,7 @@
},
"test": {
"inputs": ["default", "^default", "noMarkdown", "^noMarkdown"],
"dependsOn": ["^test"],
"dependsOn": ["^test", "^build"],
"outputs": ["{projectRoot}/coverage"],
"cache": true
},
Expand Down Expand Up @@ -152,6 +153,20 @@
"typecheckTargetName": "typecheck"
},
"include": ["packages/**/**/*", "e2e/**/**/*"]
},
{
"plugin": "@nx/vite/plugin",
"options": {
"buildTargetName": "vite:build",
"testTargetName": "vite:test",
"serveTargetName": "vite:serve",
"devTargetName": "vite:dev",
"previewTargetName": "vite:preview",
"serveStaticTargetName": "vite:serve-static",
"typecheckTargetName": "vite:typecheck",
"buildDepsTargetName": "vite:build-deps",
"watchDepsTargetName": "vite:watch-deps"
}
}
],
"parallel": 1,
Expand All @@ -161,7 +176,6 @@
},
"generators": {
"@nx/js:library": {
"publishable": true,
"outDir": "{projectRoot}/dist",
"bundler": "tsc",
"linter": "eslint",
Expand Down
45 changes: 24 additions & 21 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
{
"name": "ping-javascript-sdk",
"version": "0.0.0",
"private": true,
"description": "Ping JavaScript SDK",
"packageManager": "[email protected]+sha512.68046141893c66fad01c079231128e9afb89ef87e2691d69e4d40eee228988295fd4682181bae55b58418c3a253bde65a505ec7c5f9403ece5cc3cd37dcf2531",
"engines": {
"node": "^20 || ^22",
"pnpm": "9.15.9"
"homepage": "https://github.com/ForgeRock/ping-javascript-sdk#readme",
"bugs": {
"url": "https://github.com/ForgeRock/ping-javascript-sdk/issues"
},
"repository": {
"type": "git",
"url": "https://github.com/ForgeRock/ping-javascript-sdk.git"
},
"author": "ForgeRock",
"scripts": {
"build": "nx affected --target=build",
"clean": "shx rm -rf ./{coverage,dist,docs,node_modules,tmp}/ ./{packages,e2e}/*/{dist,node_modules}/ && git clean -fX -e \"!.env*,nx-cloud.env\"",
"changeset": "changeset",
"ci:release": "pnpm publish -r --no-git-checks && changeset tag",
"ci:version": "changeset version && pnpm install --no-frozen-lockfile && pnpm format",
"changeset": "changeset",
"circular-dep-check": "madge --circular .",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this for sanity, run it locally if you want to be safe and make sure we arent breaking the world.

"clean": "shx rm -rf ./{coverage,dist,docs,node_modules,tmp}/ ./{packages,e2e}/*/{dist,node_modules}/ && git clean -fX -e \"!.env*,nx-cloud.env\"",
"commit": "git cz",
"commitlint": "commitlint --edit",
"create-package": "nx g @nx/js:library",
"docs": "nx affected --target=typedoc",
"format": "pnpm nx format:write",
Expand All @@ -24,9 +31,8 @@
"serve": "nx serve",
"test": "CI=true nx affected:test",
"test:e2e": "CI=true nx affected:e2e",
"watch": "nx watch-deps",
"verdaccio": "nx local-registry",
"commitlint": "commitlint --edit"
"watch": "nx watch-deps"
},
"lint-staged": {
"*": [
Expand All @@ -36,15 +42,11 @@
"git add"
]
},
"repository": {
"type": "git",
"url": "https://github.com/ForgeRock/ping-javascript-sdk.git"
},
"author": "ForgeRock",
"bugs": {
"url": "https://github.com/ForgeRock/ping-javascript-sdk/issues"
"config": {
"commitizen": {
"path": "./node_modules/cz-conventional-changelog"
}
},
"homepage": "https://github.com/ForgeRock/ping-javascript-sdk#readme",
"devDependencies": {
"@changesets/changelog-github": "^0.5.0",
"@changesets/cli": "^2.27.9",
Expand Down Expand Up @@ -80,7 +82,7 @@
"@types/node": "22.13.16",
"@typescript-eslint/typescript-estree": "8.23.0",
"@typescript-eslint/utils": "^8.13.0",
"@vitest/coverage-v8": "3.0.4",
"@vitest/coverage-v8": "^3.0.5",
"@vitest/ui": "3.0.4",
"conventional-changelog-conventionalcommits": "^8.0.0",
"cz-conventional-changelog": "^3.3.0",
Expand All @@ -97,6 +99,7 @@
"jsdom": "26.0.0",
"jsonc-eslint-parser": "^2.1.0",
"lint-staged": "^15.0.0",
"madge": "8.0.0",
"npm-cli-login": "^1.0.0",
"nx": "20.5.0",
"playwright": "^1.47.2",
Expand All @@ -119,10 +122,10 @@
"vitest": "3.0.5",
"vitest-canvas-mock": "^0.3.3"
},
"config": {
"commitizen": {
"path": "./node_modules/cz-conventional-changelog"
}
"packageManager": "[email protected]+sha512.68046141893c66fad01c079231128e9afb89ef87e2691d69e4d40eee228988295fd4682181bae55b58418c3a253bde65a505ec7c5f9403ece5cc3cd37dcf2531",
"engines": {
"node": "^20 || ^22",
"pnpm": "9.15.9"
},
"nx": {
"includedScripts": []
Expand Down
3 changes: 2 additions & 1 deletion packages/davinci-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"test:watch": "pnpm nx nxTest --watch"
},
"dependencies": {
"@forgerock/javascript-sdk": "4.7.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed sdk dependency!

"@forgerock/effects": "workspace:*",
"@forgerock/shared-types": "workspace:*",
"@reduxjs/toolkit": "catalog:",
"immer": "catalog:"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/davinci-client/src/lib/client.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { davinciApi } from './davinci.api.js';
import { configSlice } from './config.slice.js';
import { wellknownApi } from './wellknown.api.js';

import type { ActionTypes, RequestMiddleware } from '@forgerock/shared-types';
/**
* Import the DaVinciRequest types
*/
Expand All @@ -31,7 +32,6 @@ import type {
import type { InitFlow, Updater, Validator } from './client.types.js';
import { returnValidator } from './collector.utils.js';
import { authorize } from './davinci.utils.js';
import type { RequestMiddleware } from './effects/request.effect.types.js';

/**
* Create a client function that returns a set of methods
Expand All @@ -41,12 +41,12 @@ import type { RequestMiddleware } from './effects/request.effect.types.js';
* @param {ConfigurationOptions} options - the configuration options for the client
* @returns {Observable} - an observable client for DaVinci flows
*/
export async function davinci({
export async function davinci<ActionType extends ActionTypes = ActionTypes>({
config,
requestMiddleware,
}: {
config: DaVinciConfig;
requestMiddleware?: RequestMiddleware[];
requestMiddleware?: RequestMiddleware<ActionType>[];
Comment on lines +44 to +49
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious what this sort of typing means (ActionType extends ActionTypes = ActionTypes)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a generic, so ActionType is like a type variable. extends in this case means it kind of matches this type but maybe not fully.

So this allows for a little tighter type narrowing over the type. If I just pass in 'DavinciStart' instead of the entire ActionTypes union, typescript will know we are only operating on 'DavinciStart' and we arent actually using the rest of the types in the union of ActionType.

I called it ActionType for a more descriptive name, but commonly you may see T as the name of the generic

Copy link
Collaborator

Choose a reason for hiding this comment

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

what does the = ActionTypes part mean? I'm confused if this is interpreted more like (ActionType extends ActionTypes) = ActionTypes or ActionType extends (ActionTypes = ActionTypes)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry left that out, It's a default if a generic isn't passed. I kept it there for ease of use, and so that if you don't include the generic it has a safety to fallback to.

}) {
const store = createClientStore({ requestMiddleware });

Expand Down
7 changes: 3 additions & 4 deletions packages/davinci-client/src/lib/client.store.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,17 @@
*/
import { configureStore } from '@reduxjs/toolkit';

import type { ActionTypes, RequestMiddleware } from '@forgerock/shared-types';
import { configSlice } from './config.slice.js';
import { nodeSlice } from './node.slice.js';
import { davinciApi } from './davinci.api.js';
import { ErrorNode, ContinueNode, StartNode, SuccessNode } from '../types.js';
import { wellknownApi } from './wellknown.api.js';

import type { RequestMiddleware } from './effects/request.effect.types.js';

export function createClientStore({
export function createClientStore<ActionType extends ActionTypes>({
requestMiddleware,
}: {
requestMiddleware?: RequestMiddleware[];
requestMiddleware?: RequestMiddleware<ActionType, unknown>[];
}) {
return configureStore({
reducer: {
Expand Down
2 changes: 1 addition & 1 deletion packages/davinci-client/src/lib/config.types.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import { describe, expectTypeOf, it } from 'vitest';
import type { DaVinciConfig, InternalDaVinciConfig } from './config.types.js';
import type { AsyncConfigOptions } from '@forgerock/javascript-sdk/src/config/interfaces';
import type { AsyncConfigOptions } from '@forgerock/shared-types';
import type { WellknownResponse } from './wellknown.types.js';

describe('Config Types', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/davinci-client/src/lib/config.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
/**
* Import ConfigOptions type from the JavaScript SDK
*/
import type { AsyncConfigOptions } from '@forgerock/javascript-sdk/src/config/interfaces';
import type { AsyncConfigOptions } from '@forgerock/shared-types';
import { WellknownResponse } from './wellknown.types.js';

export interface DaVinciConfig extends AsyncConfigOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should responseType on line 14 be of type ResponseType from the authorization types?

Expand Down
14 changes: 8 additions & 6 deletions packages/davinci-client/src/lib/davinci.api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ import {
/**
* Import internal modules
*/
import { createAuthorizeUrl } from './authorize.utils.js';
import { initQuery } from '@forgerock/effects';
import type { RequestMiddleware } from '@forgerock/shared-types';

import { createAuthorizeUrl } from '@forgerock/effects';
import { handleResponse, transformActionRequest, transformSubmitRequest } from './davinci.utils.js';

/**
Expand All @@ -35,15 +38,14 @@ import type {
} from './davinci.types.js';
import type { ContinueNode } from './node.types.js';
import type { StartNode } from '../types.js';
import { initQuery } from './effects/request.effect.utils.js';
import { RequestMiddleware } from './effects/request.effect.types.js';
import { ActionTypes } from '@forgerock/shared-types';

type BaseQueryResponse = Promise<
QueryReturnValue<unknown, FetchBaseQueryError, FetchBaseQueryMeta>
>;

interface Extras {
requestMiddleware: RequestMiddleware[];
interface Extras<ActionType extends ActionTypes = ActionTypes, Payload = unknown> {
requestMiddleware: RequestMiddleware<ActionType, Payload>[];
}

/**
Expand Down Expand Up @@ -250,7 +252,7 @@ export const davinciApi = createApi({
clientId: state?.config?.clientId,
login: 'redirect', // TODO: improve this in SDK to be more semantic
redirectUri: state?.config?.redirectUri,
responseType: state?.config?.responseType,
responseType: state?.config?.responseType as 'code',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is casted because in our config slice, the action types responseType as a string where i have it as a union of two items. can be updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble tracing where we know what the response type is here. In config.slice it defaults to code if there is no response type sent in the action payload. It can also be code if it's explicitly set. How can we be certain the response type in start will be code?

scope: state?.config?.scope,
});
const url = new URL(authorizeUrl);
Expand Down
Loading
Loading