feat(davinci-client): introduce request middleware by cerebrl · Pull Request #171 · ForgeRock/ping-javascript-sdk

cerebrl · 2025-03-19T21:59:46Z

JIRA Ticket

Description

This introduces the feature of network or request middleware to the DaVinci Client. This allows the developer to intervene just before the request is made and make any last minutes changes to a request before it is executed by the browser.

Changeset? Yes.

changeset-bot · 2025-03-19T21:59:50Z

🦋 Changeset detected

Latest commit: ce0d18f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@forgerock/davinci-client	Minor

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

nx-cloud · 2025-03-19T22:00:17Z

View your CI Pipeline Execution ↗ for commit ce0d18f.

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

☁️ Nx Cloud last updated this comment at 2025-03-25 22:58:52 UTC

github-actions · 2025-03-20T15:30:36Z

Deployed ac7598f to https://ForgeRock.github.io/ping-javascript-sdk/pr-171/ac7598f8c02c2d5a10d143677da201273542b7b6 branch gh-pages in ForgeRock/ping-javascript-sdk

codecov-commenter · 2025-03-20T15:30:46Z

Codecov Report

Attention: Patch coverage is 51.57895% with 46 lines in your changes missing coverage. Please review.

Project coverage is 50.59%. Comparing base (b0e3df8) to head (ce0d18f).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/davinci-client/src/lib/davinci.api.ts	0.00%	27 Missing ⚠️
...kages/davinci-client/src/lib/client.store.utils.ts	0.00%	13 Missing ⚠️
packages/davinci-client/src/lib/client.store.ts	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
+ Coverage   44.22%   50.59%   +6.36%     
==========================================
  Files          30       24       -6     
  Lines        1438     1354      -84     
  Branches      175      177       +2     
==========================================
+ Hits          636      685      +49     
+ Misses        802      669     -133

Files with missing lines	Coverage Δ
...nci-client/src/lib/effects/request.effect.types.ts	`100.00% <100.00%> (ø)`
...ci-client/src/lib/effects/request.effect.unions.ts	`100.00% <100.00%> (ø)`
...nci-client/src/lib/effects/request.effect.utils.ts	`100.00% <100.00%> (ø)`
packages/davinci-client/src/types.ts	`0.00% <ø> (ø)`
packages/davinci-client/src/lib/client.store.ts	`0.45% <0.00%> (+0.45%)`	⬆️
...kages/davinci-client/src/lib/client.store.utils.ts	`0.00% <0.00%> (ø)`
packages/davinci-client/src/lib/davinci.api.ts	`0.45% <0.00%> (+0.45%)`	⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ryanbas21 · 2025-03-20T21:47:41Z

@@ -0,0 +1,9 @@
+export enum ActionTypes {


Why not just use a union? Don't really see the value in an enum here.

Suggested change

export enum ActionTypes {

export type ActionTypes = 'WELL_KNOWN'| 'DAVINCI_START' ...

Got it. This was just a relic from the old middleware design. I've updated it according to your suggestion.

ryanbas21 · 2025-03-20T21:51:57Z

+
+type NextFn = () => ModifiedFetchArgs;
+
+const a = 'a' as ActionTypes;


If we take this code and the above comment about the enum

const values = { a : 'a', b: 'b', c: 'c', reassign: 'REASSIGN', } as const type ActionTypes = typeof values[keyof typeof values];

This makes it so the type is always up to date with the values, the type becomes self-updating as long as the object is updated.

we can obviously destructure all these values out if we need them individually also.

I believe this removes all of the as casting as well.

ryanbas21 · 2025-03-20T21:54:26Z

+import type { ModifiedFetchArgs, RequestMiddleware } from './request.effect.types.js';
+
+function getActionType(endpoint: string): ActionTypes {
+  switch (endpoint) {


I also think this would just become something like

return values[endpoint] || fallback if we move off the enum and just use the as const version.

ryanbas21 · 2025-03-20T23:36:05Z

I'm not a huge fan of the effect naming convention within the folder. I imagine these files will be moved to a package effects but just feels overly verbose to me personally.

This is just sticking with the convention that's already been established in the project. All files have a <name>.<file-type>.<file-extension> pattern. Yes, it's a bit verbose, but it can be helpful when files are open within displays that may not make the containing directory obvious. Plus, it really reinforces the fact that the file type strongly defines the patterns and conventions of the file contents.

ryanbas21 · 2025-03-21T13:54:14Z

+
+    function iterator(): ModifiedFetchArgs {
+      const nextMiddlewareToBeCalled = mwareCopy.shift();
+      if (nextMiddlewareToBeCalled) nextMiddlewareToBeCalled(request, actionCopy, iterator);


So when we ultimately call this nextMiddlewareToBeCalled, this function can modify the request?

Yes, that is correct. This is almost an exact copy of what we have in the legacy SDK. This line is what calls the function that is provided by the developer.

ryanbas21 · 2025-03-21T13:56:28Z


+        if (typeof api.extra === 'function') {
+          api.extra();
+          console.log(api.endpoint);


console log

ryanbas21 · 2025-03-21T14:05:02Z

+    const response = await queryApi.applyQuery(async (fetchArgs) => await mockQuery(fetchArgs));
+
+    expect(resultFetchArgs.url.toString()).toBe('https://www.example.com/?searchParam=abc');
+    expect(resultFetchArgs.headers).toStrictEqual({ 'x-new-header': '123' });


ancheetah · 2025-03-25T20:29:28Z

should we add e2e tests for headers?

Sending a custom header should cause the request to fail
Changing the value of an allowed header in the middleware config results in a change in the request header

cerebrl · 2025-03-25T21:13:30Z

should we add e2e tests for headers?

Sending a custom header should cause the request to fail

Changing the value of an allowed header in the middleware config results in a change in the request header

Good questions!

This isn't really a feature, but a consequence of PingOne not allowing CORS configuration for custom headers. So, I'm not sure it warrants a test in-and-of-itself.
Sure, we can do that.

On PTO, reviewed, tested and approved by AJ

cerebrl force-pushed the feat_network-interceptors branch from 8838b69 to 19f4681 Compare March 20, 2025 15:27

ryanbas21 previously requested changes Mar 21, 2025

View reviewed changes

cerebrl force-pushed the feat_network-interceptors branch 3 times, most recently from 8cde118 to 77c47c8 Compare March 24, 2025 19:41

cerebrl requested a review from ryanbas21 March 24, 2025 19:41

feat(davinci-client): introduce request middleware

dc469d5

cerebrl force-pushed the feat_network-interceptors branch from 77c47c8 to dc469d5 Compare March 24, 2025 20:20

cerebrl requested a review from ancheetah March 24, 2025 20:23

test(davinci-client): headers test for middleware

ce0d18f

ancheetah approved these changes Mar 25, 2025

View reviewed changes

cerebrl merged commit 6c1a685 into main Mar 25, 2025

ryanbas21 mentioned this pull request Mar 25, 2025

Release PR #11

Merged

cerebrl deleted the feat_network-interceptors branch March 26, 2025 15:59

	export enum ActionTypes {
	export type ActionTypes = 'WELL_KNOWN'\| 'DAVINCI_START' ...


		type NextFn = () => ModifiedFetchArgs;

		const a = 'a' as ActionTypes;

Conversation

cerebrl commented Mar 19, 2025

JIRA Ticket

Description

Uh oh!

changeset-bot Bot commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

nx-cloud Bot commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ancheetah commented Mar 25, 2025

Uh oh!

cerebrl commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

changeset-bot Bot commented Mar 19, 2025 •

edited

Loading

nx-cloud Bot commented Mar 19, 2025 •

edited

Loading

github-actions Bot commented Mar 20, 2025 •

edited

Loading

codecov-commenter commented Mar 20, 2025 •

edited

Loading