Skip to content
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.

Sign up for GitHub

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

chore: add support for multiple schemas within export/digitalPlanning #629

Merged
merged 4 commits into from
Jan 29, 2025
Merged

chore: add support for multiple schemas within export/digitalPlanning #629

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
903833a
add preApp passport mappings
jessicamcinchak Jan 21, 2025
4f001be
adjust return type
jessicamcinchak Jan 21, 2025
3b39b02
add preApp tests and mocks
jessicamcinchak Jan 24, 2025
6f16516
ensure both schemas are copied to dist on build
jessicamcinchak Jan 24, 2025
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
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
"check": "tsc --project ./tsconfig.check.json && pnpm lint",
"postinstall": "pnpm i rimraf && pnpm build",
"prepare": "husky install",
"generate-types-from-schema": "cd src/export/digitalPlanning/schema/ && json2ts schema.json > types.d.ts",
"copy-json-schema": "copyfiles -f ./src/export/digitalPlanning/schema/schema.json ./dist/export/digitalPlanning/schema"
"generate-types-from-schema": "cd src/export/digitalPlanning/schemas/ && json2ts application/schema.json > application/types.d.ts && json2ts preApplication/schema.json > preApplication/types.d.ts",
Copy link
Member Author

@jessicamcinchak jessicamcinchak Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm generating two types.d.ts files now - one per schema - which makes sense for function return types, but does duplicate what the "source types" (eg digital-planning-data-schemas repo) know to be "shared" types. Now they're essentially just duplicated again

"copy-json-schema": "copyfiles -f ./src/export/digitalPlanning/schemas/application/schema.json ./dist/export/digitalPlanning/application/schema && copyfiles -f ./src/export/digitalPlanning/schemas/preApplication.json ./dist/export/digitalPlanning/preApplication/schema"
},
"dependencies": {
"@emotion/react": "^11.13.3",
Expand Down
7 changes: 5 additions & 2 deletions src/export/digitalPlanning/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import { findPublishedFlowBySessionId } from "../../requests/flow.js";
import { getSessionById, getSessionMetadata } from "../../requests/session.js";
import { ExportParams } from "../index.js";
import { DigitalPlanning } from "./model.js";
import { Application as DigitalPlanningPayload } from "./schema/types.js";
import { Application as ApplicationPayload } from "./schemas/application/types.js";
import { PreApplication as PreApplicationPayload } from "./schemas/preApplication/types.js";

interface DigitalPlanningExportParams extends ExportParams {
skipValidation?: boolean;
Expand All @@ -12,7 +13,9 @@ export async function generateDigitalPlanningPayload({
client,
sessionId,
skipValidation,
}: DigitalPlanningExportParams): Promise<DigitalPlanningPayload> {
}: DigitalPlanningExportParams): Promise<
ApplicationPayload | PreApplicationPayload
> {
Copy link
Member Author

@jessicamcinchak jessicamcinchak Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In an ideal world, I think we'd have two separate classes that each return a precise type rather than a single class that returns ApplicationPayload | PreApplicationPayload - but this approach seemed like the path of least resistance to share as many of the existing passport helper methods as possible without a massive refactor or duplication right now (eg they're private methods on the class and not easy to export/import!)

As we move forward with the "prototypeApplication" and a top-level discriminated union, we know that bigger refactor is coming soon enough anyways - this is very much an interim working solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on general approach here for sure.

If we hit any type issues with this in planx-new in the short term, we could maybe consider passing an additional argument into the class -

type DigitalPlanningExportParams = {
  client: Client;
  sessionId: string;
  skipValidation?: boolean;
  type: "preApp" | ApplictionTypeUnion; // Create type discriminator here
};

export async function generateDigitalPlanningPayload({
  client,
  sessionId,
  skipValidation,
  type,
}: DigitalPlanningExportParams): Promise
 //  Modify return type based on this
  type extends "preApp'"? PreApplicationPayload : ApplicationPayload
> {
  ...
}

Returning a union though is exactly what I would have done here - I don't think we use the return type much in PlanX at this stage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Appreciate you coming back to this one ! There's an explicit reason I haven't gone with an additional argument from the planx-new side which is the (admittedly awkward) reality that discretionary/undefined-application-type payloads are also getting routed through this function to create a generic "application.json" for submission payloads (send to email & S3).

Therefore the discriminated type exposed to planx-new wouldn't be so tidy, but have to be something like this which felt pretty meaningless!

"preApp" | ApplicationTypeUnion | string | undefined

const session = await getSessionById(client, sessionId);
if (!session) throw new Error(`Cannot find session ${sessionId}`);

Expand Down
Loading
Loading