fix: Compile each project individually #969

akash1810 · 2024-05-02T10:50:26Z

What does this change, and why?

The common package has a compile error:

TS2742: The inferred type of createPullRequest cannot be named without a reference to `octokit-plugin-create-pull-request/node_modules/@octokit/types`. This is likely not portable. A type annotation is necessary.

It looks like compiling at the root isn't granular enough, as the error wasn't reported during builds.

This change resolves the compilation error, and updates to compiling each project individually. Sadly, this lengthens the build time.

This was discovered as part, and is a required, of #965.

How has it been verified?

See the build logs.

github-actions · 2024-05-02T10:54:16Z

Deploy build 4598 of `deploy::service-catalogue` to CODE

All deployment options

From guardian/actions-riff-raff.

akash1810 · 2024-05-02T10:52:05Z

package.json

@@ -6,7 +6,8 @@
    "test": "npm run test --workspaces --if-present",
    "test-update": "jest -u",
    "synth": "npm run synth --workspace=cdk",
-    "typecheck": "tsc",
+    "pretypecheck": "npm -w common run typecheck",
+    "typecheck": "npm run typecheck --workspaces",


Its worth noting we were previously compiling each project individually (see #520). The difference now though is we're not using the --if-present flag, so the concerns mentioned in #520 do not hold.

akash1810 · 2024-05-02T10:54:19Z

package.json

@@ -6,7 +6,8 @@
    "test": "npm run test --workspaces --if-present",
    "test-update": "jest -u",
    "synth": "npm run synth --workspace=cdk",
-    "typecheck": "tsc",
+    "pretypecheck": "npm -w common run typecheck",


pretypecheck will always run before typecheck see https://docs.npmjs.com/cli/v10/using-npm/scripts#pre--post-scripts. We need to compile the shared package first, as other projects depend on it, and it doesn't look like TypeScript understands this dependency.

akash1810 · 2024-05-02T10:55:13Z

packages/common/package.json

@@ -4,7 +4,8 @@
 	"type": "module",
 	"scripts": {
 		"postinstall": "prisma generate",
-		"test": "jest --detectOpenHandles --config ../../jest.config.js --selectProjects common"
+		"test": "jest --detectOpenHandles --config ../../jest.config.js --selectProjects common",
+		"typecheck": "tsc"


When compiling the common package, write the resulting files to disk (i.e. no --noEmit flag), so that it can be used by the other projects during their compile.

akash1810 · 2024-05-02T10:56:20Z

packages/dev-environment/package.json

@@ -3,7 +3,8 @@
 	"version": "0.0.0",
 	"scripts": {
 		"start": "./script/start",
-		"test": "find . | egrep '.yml|.yaml' | xargs yamllint"
+		"test": "find . | egrep '.yml|.yaml' | xargs yamllint",
+		"typecheck": "echo 'Nothing to typecheck'"


This package has no TypeScript code, so there's nothing to do.

akash1810 · 2024-05-02T10:56:23Z

packages/diagrams/package.json

@@ -4,7 +4,8 @@
 	"description": "Diagrams as code",
 	"type": "module",
 	"scripts": {
-		"generate": "mmdc -i diagram.mmd -o output.svg "
+		"generate": "mmdc -i diagram.mmd -o output.svg ",
+		"typecheck": "echo 'Nothing to typecheck'"


This package has no TypeScript code, so there's nothing to do.

The `common` package has a compile error: ``` TS2742: The inferred type of createPullRequest cannot be named without a reference to `octokit-plugin-create-pull-request/node_modules/@octokit/types`. This is likely not portable. A type annotation is necessary. ``` It looks like compiling at the root isn't granular enough, as the error wasn't reported during builds. This change updates to compiling each project individually, so that we catch such issues at CI.

There is a compilation error currently: ``` TS2742: The inferred type of createPullRequest cannot be named without a reference to `octokit-plugin-create-pull-request/node_modules/@octokit/types`. This is likely not portable. A type annotation is necessary. ``` The return type of `composeCreatePullRequest` is a bit tricky to infer. We're (currently) only using the `html_url` property, so update `createPullRequest` to return that, and therefore the return type of `createPullRequest` becomes `Promise<string | undefined>`.

akash1810 · 2024-05-03T09:00:47Z

packages/common/src/pull-requests.ts

@@ -46,6 +52,8 @@ export async function createPullRequest(
 			files,
 		})),
 	});
+
+	return response?.data.html_url;


This is the main change here, and resolves the compilation error in the PR description.

The return type of composeCreatePullRequest is a bit tricky to infer. As we're (currently) only using the html_url property, update createPullRequest to return that, and therefore the return type of createPullRequest becomes Promise<string | undefined>.

akash1810 · 2024-05-03T09:05:54Z

packages/common/src/pull-requests.ts

 export async function createPullRequest(
 	octokit: Octokit,
-	{
+	props: CreatePullRequestOptions,


This change moves the object destructing out of the function signature, in an attempt to improve readability. This is totally subjective though!

NovemberTang

Is it worth doing a test run of snyk-integrator to make sure it still behaves as expected?

akash1810 · 2024-05-03T09:10:03Z

Is it worth doing a test run of snyk-integrator to make sure it still behaves as expected?

Absolutely! How? Is createPullRequest called on CODE, as IIUC CODE doesn't raise PRs? Is there a switch to toggle to change this?

NovemberTang · 2024-05-03T09:36:54Z

Is it worth doing a test run of snyk-integrator to make sure it still behaves as expected?

Absolutely! How? Is createPullRequest called on CODE, as IIUC CODE doesn't raise PRs? Is there a switch to toggle to change this?

Yeah there's a couple of ways to do this. You can either:

Temporarily modify the Stage env var in the console to from CODE to PROD, bypassing the flag
Deploy your branch to PROD
Remove the stage awareness in the source code, build it, and paste it into the text editor in the console

akash1810 · 2024-05-03T11:06:29Z

Deploy your branch to PROD

I've done this, with a resulting PR of #976.

@NovemberTang and I have been discussing the snyk-integrator over chat. Here's a summary for others:

In general, masquerading as PROD should be avoided as there are potentially unknown side-effects. For example, imagine there are multiple stage aware branches in the application, we'll trigger all of them when only wanting to test one. Testing on PROD isn't ideal, for similar reasons.

Mutating the lambda within the AWS console should also be avoided, as it removes the audit trail, and introduces elements of human error. Updating the lambda with a traditional deployment means all the CI checks have passed, increasing confidence.

In this specific scenario, only one piece of the application is stage aware, and that is the part we are wanting to test. Whilst there are opportunities to refactor parts of the code to create more pure functions, it is most pragmatic to test on PROD, as this PR unblocks others. We will make the refactors in follow up PRs.

akash1810 requested review from a team as code owners May 2, 2024 10:50

akash1810 commented May 2, 2024

View reviewed changes

akash1810 force-pushed the aa/tsc branch from b71fd1d to e53f49b Compare May 3, 2024 08:46

akash1810 added 2 commits May 3, 2024 09:53

akash1810 force-pushed the aa/tsc branch from e53f49b to 83ab334 Compare May 3, 2024 08:59

akash1810 commented May 3, 2024

View reviewed changes

NovemberTang reviewed May 3, 2024

View reviewed changes

akash1810 mentioned this pull request May 3, 2024

Integrate Scala, Go projects with Snyk #976

Closed

3 tasks

NovemberTang approved these changes May 3, 2024

View reviewed changes

akash1810 merged commit de489ad into main May 3, 2024
2 checks passed

akash1810 deleted the aa/tsc branch May 3, 2024 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Compile each project individually #969

fix: Compile each project individually #969

akash1810 commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

akash1810 May 2, 2024

akash1810 May 2, 2024

akash1810 May 2, 2024

akash1810 May 2, 2024

akash1810 May 2, 2024

akash1810 May 3, 2024 •

edited

Loading

akash1810 May 3, 2024 •

edited

Loading

NovemberTang left a comment

akash1810 commented May 3, 2024 •

edited by NovemberTang

Loading

NovemberTang commented May 3, 2024

akash1810 commented May 3, 2024 •

edited

Loading

fix: Compile each project individually #969

fix: Compile each project individually #969

Conversation

akash1810 commented May 2, 2024 • edited Loading

What does this change, and why?

How has it been verified?

github-actions bot commented May 2, 2024 • edited Loading

Deploy build 4598 of deploy::service-catalogue to CODE

akash1810 May 2, 2024

Choose a reason for hiding this comment

akash1810 May 2, 2024

Choose a reason for hiding this comment

akash1810 May 2, 2024

Choose a reason for hiding this comment

akash1810 May 2, 2024

Choose a reason for hiding this comment

akash1810 May 2, 2024

Choose a reason for hiding this comment

akash1810 May 3, 2024 • edited Loading

Choose a reason for hiding this comment

akash1810 May 3, 2024 • edited Loading

Choose a reason for hiding this comment

NovemberTang left a comment

Choose a reason for hiding this comment

akash1810 commented May 3, 2024 • edited by NovemberTang Loading

NovemberTang commented May 3, 2024

akash1810 commented May 3, 2024 • edited Loading

akash1810 commented May 2, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading

Deploy build 4598 of `deploy::service-catalogue` to CODE

akash1810 May 3, 2024 •

edited

Loading

akash1810 May 3, 2024 •

edited

Loading

akash1810 commented May 3, 2024 •

edited by NovemberTang

Loading

akash1810 commented May 3, 2024 •

edited

Loading