Skip to content

Conversation

@Puppo
Copy link
Contributor

@Puppo Puppo commented Jun 25, 2025

  • Move appConfig from tsd to tstyche

@Puppo Puppo force-pushed the chore/move-from-tsd-to-tstyche-appconfig branch from e70d34d to 97c5ed3 Compare June 26, 2025 16:42
@Puppo Puppo changed the base branch from main to develop June 26, 2025 16:47
@Puppo Puppo force-pushed the chore/move-from-tsd-to-tstyche-appconfig branch from 97c5ed3 to db544dc Compare June 26, 2025 16:53
@Puppo Puppo force-pushed the chore/move-from-tsd-to-tstyche-appconfig branch from db544dc to ba596ff Compare June 26, 2025 16:54
@Puppo Puppo marked this pull request as ready for review June 26, 2025 17:01
@Puppo Puppo requested a review from a team as a code owner June 26, 2025 17:01
Comment on lines +26 to +27
"test:types:tsd": "find packages -name \"*.test-d.ts\" -exec dirname {} \\; | sed 's|packages/||' | sort -u | xargs -I {} tsd packages/{}",
"test:types:tstyche": "find packages -name \"*.tst.ts\" -exec dirname {} \\; | sed 's|packages/||' | sort -u | xargs -I {} tstyche packages/{}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one allows us to keep both systems during the migration, and run the tests only where they are available for that framework.

Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

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

A quick note, you can use .not.toBeCallableWith() instead of // @ts-expect-error. The matcher indicates the purpose of a test somewhat better.

Comment on lines +80 to +81
// @ts-expect-error - fetchData must be an object
appConfig({ ...options, fetchData: "not-an-object" });
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-expect-error - fetchData must be an object
appConfig({ ...options, fetchData: "not-an-object" });
expect(appConfig).type.not.toBeCallableWith({
...options,
fetchData: "not-an-object",
});

Comment on lines +83 to +95
appConfig({
...options,
fetchData: {
config: {
// @ts-expect-error - Application must be a string
ApplicationIdentifier: 123,
// @ts-expect-error - Configuration must be a string
ConfigurationProfileIdentifier: 123,
// @ts-expect-error - Environment must be a string
EnvironmentIdentifier: 123,
},
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
appConfig({
...options,
fetchData: {
config: {
// @ts-expect-error - Application must be a string
ApplicationIdentifier: 123,
// @ts-expect-error - Configuration must be a string
ConfigurationProfileIdentifier: 123,
// @ts-expect-error - Environment must be a string
EnvironmentIdentifier: 123,
},
},
});
expect(appConfig).type.not.toBeCallableWith({
...options,
fetchData: {
config: {
ApplicationIdentifier: 123,
ConfigurationProfileIdentifier: 123,
EnvironmentIdentifier: 123,
},
},
});

Comment on lines +97 to +103
appConfig({
...options,
fetchData: {
// @ts-expect-error - config must contain Application, ClientId, Configuration and Environment
config: {},
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
appConfig({
...options,
fetchData: {
// @ts-expect-error - config must contain Application, ClientId, Configuration and Environment
config: {},
},
});
expect(appConfig).type.not.toBeCallableWith({
...options,
fetchData: {
config: {},
},
});

"test:sast:trufflehog": "trufflehog filesystem --only-verified --log-level=-1 ./",
"test:types": "ls packages | xargs -I {} tsd packages/{}",
"test:types:tsd": "find packages -name \"*.test-d.ts\" -exec dirname {} \\; | sed 's|packages/||' | sort -u | xargs -I {} tsd packages/{}",
"test:types:tstyche": "find packages -name \"*.tst.ts\" -exec dirname {} \\; | sed 's|packages/||' | sort -u | xargs -I {} tstyche packages/{}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"test:types:tstyche": "find packages -name \"*.tst.ts\" -exec dirname {} \\; | sed 's|packages/||' | sort -u | xargs -I {} tstyche packages/{}",
"test:types:tstyche": "tstyche",

Copy link
Contributor

@mrazauskas mrazauskas Jun 26, 2025

Choose a reason for hiding this comment

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

If you stick with the *.tst.ts pattern, TSTyche will handle that.

import type { Context as LambdaContext } from "aws-lambda";
import { captureAWSv3Client } from "aws-xray-sdk";
import { expect, test } from "tstyche";
import appConfig, { appConfigReq } from ".";
Copy link
Contributor

Choose a reason for hiding this comment

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

How can it be that extension less imports are still allowed? Ah.. The root ./tsconfig.json makes this work. It defaults to CJS resolution. Is that useful for any other tooling?

I see two options:

  • npm run test:types:tstyche -- --tsconfig ignore, simply use TSTyche defaults;
  • extend the root TSConfig from @tsconfig/node20, since Node.js 20 is the lowest supported version

Copy link
Contributor

Choose a reason for hiding this comment

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

The extension less imports should fail after badad62. (Many thanks to @willfarrell!)

cacheKey: "some-key",
cacheExpiry: 60 * 60 * 5,
setToContext: false,
} as const;
Copy link
Contributor

@mrazauskas mrazauskas Jun 26, 2025

Choose a reason for hiding this comment

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

Perhaps? And remember to import the AppConfigOptions type:

Suggested change
} as const;
} satisfies AppConfigOptions;

@willfarrell
Copy link
Member

#1384 has been merged in. The new approach looks way easier to understand. With no comments against moving forward, I would say feel free to move forward with all middlewares if you like,

@willfarrell willfarrell marked this pull request as draft July 28, 2025 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants