feat: add-effects-package #224
Conversation
🦋 Changeset detectedLatest commit: 05fd6f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit 05fd6f1.
☁️ Nx Cloud last updated this comment at |
|
Deployed 940cda6 to https://ForgeRock.github.io/ping-javascript-sdk/pr-224/940cda6f49adecd791043619da3dc9df00ab457a branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #224 +/- ##
==========================================
+ Coverage 45.18% 47.24% +2.05%
==========================================
Files 33 29 -4
Lines 1547 1469 -78
Branches 186 147 -39
==========================================
- Hits 699 694 -5
+ Misses 848 775 -73
🚀 New features to boost your workflow:
|
| @@ -181,7 +186,9 @@ describe('initQuery function', () => { | |||
|
|
|||
| // @ts-expect-error - Intentionally testing an unknown action type | |||
| const queryApi = initQuery(fetchArgs, endpoint).applyMiddleware(requestMiddleware); | |||
| const response = await queryApi.applyQuery(async (fetchArgs) => await mockQuery(fetchArgs)); | |||
| const response = await queryApi.applyQuery( | |||
| async (fetchArgs: FetchArgs) => await mockQuery(fetchArgs), | |||
There was a problem hiding this comment.
for whatever reason, fetchArgs in my IDE was properly inferred, but the typecheck command was unhappy (i couldnt figure out at that time what was causing it though) worth looking into more though.
ryanbas21
force-pushed
the
effects-package
branch
from
13b3ef0 to
7c42fee
Compare
| @@ -25,7 +26,7 @@ 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'; | |||
| import { ActionTypes } from '@forgerock/effects/unions'; | |||
There was a problem hiding this comment.
I'm not quite sure I'm a fan of using the name unions separately from types. From the a user of the library's perspective, I'm not sure how intuitive it is to import from unions to get the ActionTypes. I would prefer to place all type-related items, whether they are technically a type, interface, union, enum, generic ... in /types.
There was a problem hiding this comment.
Huh, I guess I'm the one who initially use the convention of the file type .unions.ts in the code base, but I guess it's because it was originally an enum. Either way, I don't think there's any value to using this convention and would prefer to combine the union types into the our standard .types.ts file convention.
| applyMiddleware(middleware: RequestMiddleware[] | undefined) { | ||
| applyMiddleware( | ||
| middleware: RequestMiddleware<ActionTypes, ModifiedFetchArgs['body']>[] | undefined, | ||
| ) { | ||
| // Iterates and executes provided middleware functions | ||
| // Allow middleware to mutate `request` argument | ||
| const runMiddleware = middlewareWrapper(modifiedRequest, { type: actionTypes[endpoint] }); |
There was a problem hiding this comment.
Oh, I see ... the way we did this "union" was by creating it from an actual used object. This does have the downside of it now being both a type and runtime code ... kind of like an enum. Can we rethink this? If we go unions, let's do it in a way were the union is just a type and isn't dependent on runtime code.
If the union needs the actual runtime object, and we put in the the same file as the other code, we run the risk of circular dependencies, so that's not good either.
There was a problem hiding this comment.
Do we need the object value? Or are we really just looking for the type?
We can define the union inline, or simply not export the object.
There was a problem hiding this comment.
The current code is beneficial though. There are some use cases that enums don't support that this now does.
The current line 65, is one example. The type can be used either like actionTypes[endpoint] or by passing in the string equivalent. Where an enum you must use the enum.
If we don't want to export the object, we can keep exporting the type, or we can remove the object and inline the union like 'a' | 'b' | 'c'
If the union needs the actual runtime object, and we put in the the same file as the other code, we run the risk of circular dependencies, so that's not good either.
I don't understand this line fully. Maybe i'm not visualizing this in my head well. As the code stands now, in a single file, theres no way it can become circular, because the only imports in the types file are RTK related.
There was a problem hiding this comment.
I should correct myself - any file can be a part of a circular import but I don't see how this pattern itself is the problem. but I could be missing something.
There was a problem hiding this comment.
Oh, sorry, let me rephrase my original statement. I have no issue with us using the union pattern. My issue lies with the fact that we have a .unions.ts file that contains both an runtime object AND a type. My ask is that we re-evaluate the fact that this file has both, not that we go back to enums.
The circular dependency will come about if we move the runtime object out into the file that uses it, the .effects.ts, and move the type into the .types.ts file. That would mean the .types.ts file would have to import the object from .effects.ts for the object, and the .effects.ts file would have to import the type from .types.ts.
There was a problem hiding this comment.
@cerebrl As it stands now, I have moved everything into the types file. the types file only imports RTK types it looks like
There was a problem hiding this comment.
We can, on the other hand, introduce this as a new convention into the project's architectural design. We could just keep this .unions.ts file as a way to handle this new paradigm. But, I'd like your opinion here. My biggest concern is really that we don't expose this pattern to our customers. So, the type that's derived from the object is exported publicly with the rest of the types in the /types module.
There was a problem hiding this comment.
I'm so lost @cerebrl That is what I'm doing...everything is in the types file? Should we hop on a call?
|
Have the authorize utils been moved from davinci-client to effects package yet? |
Good call out. I think I should wait until your PR is merged (or we can just rebase these into 1 pr) because your PR moves the PKCE code. I could do it, and install the legacy sdk in the package but i'm not sure that is worth doing since it would be immediately changed. |
ryanbas21
force-pushed
the
effects-package
branch
2 times, most recently
from
c6b4e74 to
f021341
Compare
Refactored some types to allow for better passing of types through and narrowing.
Refactor of the union.ts file to a single /types file. removed union export.
ported over many features of the SDK, and did a best effort to maintain API surface.
ryanbas21
force-pushed
the
effects-package
branch
from
3795dfe to
9c6756b
Compare
.changeset/lemon-rocks-dream.md
Outdated
There was a problem hiding this comment.
We can probably omit this changeset
.changeset/slick-cougars-smoke.md
Outdated
There was a problem hiding this comment.
Probably can omit this as well
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Will add changelogs to these packages and update here.
| 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); |
There was a problem hiding this comment.
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
nx.json
Outdated
| @@ -69,7 +70,7 @@ | |||
| }, | |||
| "test": { | |||
| "inputs": ["default", "^default", "noMarkdown", "^noMarkdown"], | |||
| "dependsOn": ["^test"], | |||
| "dependsOn": ["^test", "^build", "^build", "^build"], | |||
There was a problem hiding this comment.
not sure why this happened, fixing locally now.
| "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 .", |
There was a problem hiding this comment.
Added this for sanity, run it locally if you want to be safe and make sure we arent breaking the world.
| @@ -22,7 +22,8 @@ | |||
| "test:watch": "pnpm nx nxTest --watch" | |||
| }, | |||
| "dependencies": { | |||
| "@forgerock/javascript-sdk": "4.7.0", | |||
There was a problem hiding this comment.
removed sdk dependency!
| @@ -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', | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| @@ -0,0 +1,11 @@ | |||
| # effects | |||
There was a problem hiding this comment.
Should probably have something more in all of these later
There was a problem hiding this comment.
I understand this file and the logger.ts will change, its just written to move foward, i'm open to changing it
| @@ -0,0 +1,43 @@ | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| const callbackType = { | |||
There was a problem hiding this comment.
Ported for compatibility.
There was a problem hiding this comment.
ignoring the unused variable because we arent using the callbackType object anywhere.
There was a problem hiding this comment.
Actually now that I think of this, this would technically be breaking....should we go back to enum?
| * step in an authentication tree. | ||
| */ | ||
| export interface StepOptions extends ConfigOptions { | ||
| query?: Record<string, string>; |
There was a problem hiding this comment.
I left StringDict....its the same type though
| * @param scope The scope of the authorization request | ||
| */ | ||
| export type ResponseType = 'code' | 'token'; | ||
| export interface GetAuthorizationUrlOptions extends ConfigOptions { |
There was a problem hiding this comment.
Is this the best place for this interface? It's specific to the getAuthorizeUrl function so maybe it should live with it in authorization.types.ts in the effects package.
There was a problem hiding this comment.
Yeah, it may be able to live there, I was moving a lot of stuff so I was trying to lower things down in the tree and revisit them once it was all working
| @@ -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'>[] = [ | |||
There was a problem hiding this comment.
why not use ActionTypes here?
RequestMiddleware<ActionTypes>
| export async function davinci<ActionType extends ActionTypes = ActionTypes>({ | ||
| config, | ||
| requestMiddleware, | ||
| }: { | ||
| config: DaVinciConfig; | ||
| requestMiddleware?: RequestMiddleware[]; | ||
| requestMiddleware?: RequestMiddleware<ActionType>[]; |
There was a problem hiding this comment.
Just curious what this sort of typing means (ActionType extends ActionTypes = ActionTypes)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
what does the = ActionTypes part mean? I'm confused if this is interpreted more like (ActionType extends ActionTypes) = ActionTypes or ActionType extends (ActionTypes = ActionTypes)
There was a problem hiding this comment.
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.
| @@ -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 { | |||
There was a problem hiding this comment.
Should responseType on line 14 be of type ResponseType from the authorization types?
Refactored some types to allow for better passing of types through and narrowing.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-3795
Description
Added effects package, and refactored a type so that there is better narrowing when passing in values. if its too hectic, i'm open to removing the generic. I tried to make it so theres sane defaults / its not super required, although I may have missed areas if we want that behavior.