Refactor lectern-client Rest Module to work in Browser or on Server by joneubank · Pull Request #247 · overture-stack/lectern

joneubank · 2025-05-20T21:41:07Z

Description

This includes the final changes needed for the lectern-client to run inside a browser, while still running succesfully on the server.

This requires removing two dependencies that only work inside NodeJS (winston and node-fetch), and refactoring the modules that were using them. The client's rest module was re-written from scratch to provide functions to list all dictionaries on a server, and to fetch a single Dictionary either by ID or by Name and Version.

Future work is needed to also include a getDiff or getSchema/getField. All the POST/PUT endpoints that modify data will also require auth integration, and these have been left for future tasks.

This is a breaking change in that it changes which fetch functions were available in the client, but it should also unblock any browser integrations.

Server CORS

In addition to the client changes, the Lectern Server needed to have CORS configuration added so that requests from a browser would succeed. The server has the Express library updated to latest and the expressjs/cors middleware added. An environment variable is now provided that will allow configuration of the allowed CORS domains. By default, CORS will reject all domains, so no security changes for existing servers.

Fixes

Make the client usable in browser and server #213

Type of change

Please choose only the relevant options and remove the others.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

Server has unit tests added for CORS functionality
Client has been tested through REPL import of the built library and integration tests run against a local running Lectern Server to list and get dictionaries.

Future Work

Lyric uses the client, and will need to be updated to use the new rest method: https://github.com/overture-stack/lyric/blob/main/packages/data-provider/src/external/lecternClient.ts#L18

Checklist:

You do not need to fullfill all requirements of this checklist, select all that apply:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

ciaranschutte

couple of comments. tests + validation are looking great.

ciaranschutte · 2025-05-21T00:43:52Z

@@ -22,11 +22,15 @@
 	"homepage": "https://github.com/overture-stack/lectern#readme",
 	"devDependencies": {
 		"@types/body-parser": "^1.19.5",


remove types for unused body-parser package

ciaranschutte · 2025-05-21T00:44:46Z

+		"@types/chai-http": "^4.2.4",
+		"@types/cors": "^2.8.18",
 		"@types/errorhandler": "0.0.32",
 		"@types/express": "^4.17.21",


upgraded express package but not it's types

This helped me spot an underlying type issue, thank you.

ciaranschutte · 2025-05-21T00:49:19Z

 	"dependencies": {
 		"@overture-stack/lectern-dictionary": "workspace:^",
 		"@overture-stack/lectern-validation": "workspace:^",
+		"axios": "^1.7.2",


comment.

web standard fetch is available in node
axios is overkill
aligns with removing things we don't need (body-parser removes in preference for native express api)

fetch in any of its flavors doesn't include cancelling (you'd have to instantiate an AbortController separately), which is something we've been meaning to start implementing consistently.

does that add any nuance to this choice?

ciaranschutte · 2025-05-21T00:49:50Z

+// export const fetchDiffAndAnalyze = async (serviceUrl: string, name: string, fromVersion: string, toVersion: string) => {
+// 	// const changes = await restClient.fetchDiff(serviceUrl, name, fromVersion, toVersion);
+// 	return analyzeChanges(changes);
+// };


delete or add comment explaining why code is commented out

ciaranschutte · 2025-05-21T00:53:59Z

+const allAvailableDictionariesResult = await lectern.rest.listDictionaries(lecternUrl);
+const filteredByNameDictionariesResult = await lectern.rest.listDictionaries(lecternUrl, {name: dictionaryName});
+
+const exampleDicionaryResult = await lectern.rest.fetchDiff(lecternUrl, {name: dictionaryName, version: dictionaryVersion});


is fetchDiff removed? description of PR says

"Future work is needed to also include a getDiff or getSchema/getField. All the POST/PUT endpoints that modify data will also require auth integration, and these have been left for future tasks."

justincorrigible · 2025-05-21T11:28:19Z

 # AUTH_ENABLED=false
 # EGO_API=
 # SCOPE=
+# # CORS allowed origins can be a comma separated list of the allowed domains.


does this parse spacing? e.g. a,b,c vs a, b, c

no, this takes whitespace as part of the value. very simple implementation

justincorrigible · 2025-05-21T11:29:15Z

+pnpm nx build @overture-stack/lectern-server
+```
+or
+```shell
+pnpm build


what's the difference, if any? otherwise, line 43 should suffice as "lower complexity" choice

justincorrigible · 2025-05-21T11:29:37Z

+pnpm nx start @overture-stack/lectern-server
+```
+or
+```shell
+pnpm start


same here. difference? otherwise, simpler is best

justincorrigible · 2025-05-21T11:38:32Z

+| -------------------- | ------------------------------ | ------------ | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| OPENAPI_PATH         | No                             | String       | `/api-docs` | Path to Swagger UI with API documentation.                                                                                                              |
+| PORT                 | No                             | Number       | `3000`      | Port Lectern Server API will listen to.                                                                                                                 |
+| CORS_ALLOWED_ORIGINS | No                             | String[]     | -           | List of domains that will be allowed by CORS. Multiple domains can be listed, separated by commas. Example: `http://localhost:5173,https://example.com` |


ah, looks like this is the answer to my earlier question. perhaps examples would be useful in the .env.example? (unless the intent was to create a .env.template instead)

Suggested change

| CORS_ALLOWED_ORIGINS | No | String[] | - | List of domains that will be allowed by CORS. Multiple domains can be listed, separated by commas. Example: `http://localhost:5173,https://example.com` |

| CORS_ALLOWED_ORIGINS | No | String[] | - | List of domains that will be allowed by CORS. Multiple domains can be listed, separated by commas. No spaces. Example: `http://localhost:5173,https://example.com` |

I'll add the trim thing instead.

justincorrigible · 2025-05-21T11:48:31Z

 		};
-		return res.status(200).send(resBody);
+		res.status(200).send(resBody);
+		return;


naive question: return is implicitly undefined, so it must be to end the condition. what's the purpose of putting it separately from the response? surely return res... has the exact same effect without looking unnecessarily verbose, and following a well established expressjs pattern.

The updated version of Express's request handlers have a return type that doesn't allow returning the response function, so the existing returns were breaking build. I add the explicit return by habit to end the function after sending the response to prevent accidentally modifying a response that's already been sent (which causes exceptions in express).

ugh Express! le why you le break? 😞
it was a nice pattern! are you sure it's not just their TS implementation that's the problem?

Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>

…verture-stack#247) * Enable CORS for server by configuration * Refactor client rest module to work in browser or server * Update client README for latest Rest module function usage * Remove distracting comments from test * Remove unused code and references to old dependencies * Upgrade @types packages and update affected code * Remove vestigial change analysis code imported from legacy client * Organize imports, format code example in readme * Trim cors allowed origins values Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com> * Remove tests associated with removed code --------- Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>

joneubank added 3 commits May 20, 2025 15:42

Enable CORS for server by configuration

edc220e

Refactor client rest module to work in browser or server

54c19b5

Update client README for latest Rest module function usage

7202b9f

joneubank requested review from MitchellShiell, SaqAsh, justincorrigible and leoraba May 20, 2025 21:41

joneubank added 2 commits May 20, 2025 17:43

Remove distracting comments from test

d9a91cf

Remove unused code and references to old dependencies

6698fd8

ciaranschutte approved these changes May 21, 2025

View reviewed changes

joneubank added 3 commits May 20, 2025 22:28

Upgrade @types packages and update affected code

611d16d

Remove vestigial change analysis code imported from legacy client

e3373c9

Organize imports, format code example in readme

f855884

justincorrigible reviewed May 21, 2025

View reviewed changes

Comment thread apps/server/src/config/appConfig.ts Outdated

justincorrigible reviewed May 21, 2025

View reviewed changes

Trim cors allowed origins values

b714db8

Co-authored-by: Anders Richardsson <2107110+justincorrigible@users.noreply.github.com>

joneubank force-pushed the fix/client-for-browser branch from c061713 to b714db8 Compare May 21, 2025 17:43

Remove tests associated with removed code

2233d73

joneubank merged commit 9b46a9c into main May 21, 2025
2 checks passed

joneubank deleted the fix/client-for-browser branch May 21, 2025 18:51

joneubank mentioned this pull request May 22, 2025

Release Candidate 2.0.0-beta.4 #250

Merged

	\| CORS_ALLOWED_ORIGINS \| No \| String[] \| - \| List of domains that will be allowed by CORS. Multiple domains can be listed, separated by commas. Example: `http://localhost:5173,https://example.com` \|
	\| CORS_ALLOWED_ORIGINS \| No \| String[] \| - \| List of domains that will be allowed by CORS. Multiple domains can be listed, separated by commas. No spaces. Example: `http://localhost:5173,https://example.com` \|

Conversation

joneubank commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Server CORS

Fixes

Type of change

How Has This Been Tested?

Future Work

Checklist:

Uh oh!

ciaranschutte left a comment

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!

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!

Uh oh!

justincorrigible May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joneubank commented May 20, 2025 •

edited

Loading

justincorrigible May 21, 2025 •

edited

Loading