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.

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

integ-runner: integ-runner implictly depends on ts-node #23710

Open
tmokmss opened this issue Jan 17, 2023 · 9 comments
Open

integ-runner: integ-runner implictly depends on ts-node #23710

tmokmss opened this issue Jan 17, 2023 · 9 comments
Labels
@aws-cdk/integ-runner bug This issue is a bug. cli Issues related to the CDK CLI effort/medium Medium work item – several days of effort p2

Comments

@tmokmss
Copy link
Contributor

tmokmss commented Jan 17, 2023

Describe the bug

Hi, I found a tiny minor issue regarding integ-runner. It'd be great if you check it when you get a chance.

Starting from 2.60.0 (#22058), it seems @aws-cdk/integ-runner implicitly depends on ts-node.

typescript: ['node -r ts-node/register {filePath}', ['^integ\\.(?!.*\\.d\\.ts$).*\\.ts$']],

Without ts-node, the error below happens:

$ yarn integ-runner --update-on-failed
yarn run v1.22.17
Verifying integration test snapshots...

node:internal/modules/cjs/loader:988
  throw err;
  ^

Error: Cannot find module 'ts-node/register'
Require stack:
- internal/preload
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:985:15)
    at Function.Module._load (node:internal/modules/cjs/loader:833:27)
    at Module.require (node:internal/modules/cjs/loader:1057:19)
    at Module._preloadModules (node:internal/modules/cjs/loader:1332:12)
    at loadPreloadModules (node:internal/bootstrap/pre_execution:583:5)
    at prepareMainThreadExecution (node:internal/bootstrap/pre_execution:95:3)
    at node:internal/main/run_main_module:9:1 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ 'internal/preload' ]
}
  ERROR      integ.nodejs-build 0.088s
      Command exited with status 1

As for usual CDK apps, ts-node is installed by default so it shouldn't be a big problem. In CDK construct library templates generated by projen, however, ts-node is not installed by default so this library behaves somewhat confusing.
Isn't it be more clear if it is explicitly specified in package.json of integ-runner as peerDependency or Dependency?

Expected Behavior

When installing @aws-cdk/integ-runner, ts-node is installed automatically or at least suggested as a peer dependency.

It will help us a lot if there is more clear way to know the requirements of ts-node, or it is installed automatically.

Current Behavior

We get an error without ts-node. Also we need to carefully inspect the error and manually install ts-node.

Reproduction Steps

Use integ-runner for *integ.ts files without installing ts-node.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.60.0

Framework Version

2.60.0

Node.js Version

v16.18.1

OS

macOS

Language

Typescript

Language Version

No response

Other information

No response

@tmokmss tmokmss added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 17, 2023
@pahud pahud added p1 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 16, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Mar 6, 2023

Hi @tmokmss

That's a good question. To me it's in the same realm as running integ-runner --language=python which also doesn't come with its own dependencies.

Peer Dependencies these aren't much different from regular dependencies any more, in the sense that they are automatically installed. So adding ts-node would be quite the extra load.

Maybe we should check for it's presence and print a nice error?

@tmokmss
Copy link
Contributor Author

tmokmss commented Mar 7, 2023

Hi @mrgrain, thanks. I agree with that adding ts-node as a dependency is technically not a proper solution, and raising a nicer error is enough for the problem. At the same time, I also tend to think installing ts-node automatically would be more user-friendly and a huge benefit since I believe most users will use integ-runner with TypeScript. Also, integ-runner is usually added as a dev dependency so it shouldn't much affect users who cares CDK package size.

@mrgrain mrgrain added p2 and removed p1 labels Mar 7, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Mar 7, 2023

At some point I had the idea of checking cdk.json for the app string. But the parsing seemed very unreliable.

@tmokmss I'm curious - how did you come about finding this out? Did you add integ-runner to a JS only app?
Really just trying to understand the scenario here. I would have thought that anyone running TS integ tests would already have ts-node installed.

PS: Downgrading to p2 since this is an experimental package. But it's on our backlog for integ-runner.

@tmokmss
Copy link
Contributor Author

tmokmss commented Mar 7, 2023

@mrgrain Sure:) I found this when developing a CDK construct library by using projen (example). The projen template does not include ts-node by default (like the aws-cdk repo), so just adding @aws-cdk/integ-runner won't work.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 7, 2023

Makes sense. I guess the projen template wants you to first compile your code to JS?

integ-runner does account for that, if it fnds a js file of the same name or run with --language=javascript

@tmokmss
Copy link
Contributor Author

tmokmss commented Mar 7, 2023

yeah that's why I set an additional build step here; iirc yarn build only builds files in the src directory by default so I needed this line (to build .ts files in the test directory).

I believe some guideline about using integ-runner for a CDK construct library will also help a lot (maybe after graduation from experimental). I'm also not sure how we can use the latest integ-runner with some older aws-cdk-lib package (related question: #23711).

@mrgrain
Copy link
Contributor

mrgrain commented Mar 7, 2023

Absolutely! And native projen support is definitely on my list 😅

AwsCdkConstructLibrary should probably add ts-node as an explicit dependency as well. It's a quirk I haven't noticed that yet. ts-node gets added when you use a .projenrc.ts file (as a opposed to .js).

@tmokmss
Copy link
Contributor Author

tmokmss commented Mar 7, 2023

Cool, I'm looking forward to it ❤️
I didn't know about .projenrc.ts as npx projen new awscdk-construct just initializes with a projenrc.js...

@mrgrain
Copy link
Contributor

mrgrain commented Mar 7, 2023

I didn't know about .projenrc.ts as npx projen new awscdk-construct just initializes with a projenrc.js...

Yes, I hate this. But haven't found a good way yet to change the default without breaking everyone :(

@pahud pahud added the cli Issues related to the CDK CLI label Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/integ-runner bug This issue is a bug. cli Issues related to the CDK CLI effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

3 participants