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

[Feature Request] Accept a Typescript file as configuration for the Merge CLI for monocart-reporter #145

Closed
edumserrano opened this issue Sep 17, 2024 · 23 comments

Comments

@edumserrano
Copy link

Would it be possible to accept a .ts file as the configuration file passed to the merge CLI?

Would it be possible to do something like:

ts-node can register the typescript file to be transpiled on runtime and there by allowing us to use the typescript file in JS file

As described in How to use Typescript files inside Javascript file?.

I'm out of my depth here so perhaps I'm just saying nonsense...

If it helps understand why I'm asking this:

It's because I have typescript for the Playwright configuration file and I'd like to share code between the Playwright configuration and the merge CLI configuration and have the shared code in Typescript instead of Javascript.

@cenfun
Copy link
Owner

cenfun commented Sep 18, 2024

The default config files are following

  • 'mr.config.js'
  • 'mr.config.cjs'
  • 'mr.config.mjs'
  • 'mr.config.json'
  • 'mr.config.ts'

But the mr.config.ts requires preloading for the ts execution module.
The popular libs are ts-node or tsx

for example, create a mr.config.ts with options and run CLI following

cross-env NODE_OPTIONS="--loader ts-node/esm --no-warnings" monocart merge ./docs/shard*/index.json

Unfortunately, it's not working currently, I will fix it in the next patch.

@edumserrano
Copy link
Author

edumserrano commented Sep 18, 2024

In your example above I assume you are missing the -c parameter with the .ts config file for the CLI? Or perhaps it will try to use a mr.config.ts if it exists even if it's not passed as the -c option?

@cenfun
Copy link
Owner

cenfun commented Sep 18, 2024

I forgot install typescript. Please try following steps:

  • install
npm i ts-node typescript cross-env -D
cross-env NODE_OPTIONS="--loader ts-node/esm --no-warnings" monocart merge ./docs/shard*/index.json

cross-env will support Windows/Linux/MacOS

Yes, it is not necessary to specify the config file path with -c parameter, It will search for the default config file in the cwd
The default config file are (In order of priority)

  • 'mr.config.js'
  • 'mr.config.cjs'
  • 'mr.config.mjs'
  • 'mr.config.json'
  • 'mr.config.ts'

@edumserrano
Copy link
Author

edumserrano commented Sep 20, 2024

I tried your suggestion but without any luck:

npx cross-env NODE_OPTIONS="--loader ts-node/esm --no-warnings" monocart merge ./shards/**/monocart-report.json -c .\playwright.monocart-reporter-merge.ts

Resulted in:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": Cannot use import statement outside a module

The contents of the playwright.monocart-reporter-merge.ts merge CLI config file are:

import path from "node:path";

const reportersOutputDir = path.resolve("./test-reporters/playwright");
// commented out the rest of the file since it errors with just this due to the import statement.

Apologies if this is a real simple type of error but I'm not very familiar with the frontend ecosystem and I don't know what I should do to get this to work. Any thoughts?

Also, as suggested by the article I linked in the initial post, would it not be possible for the merge CLI to use the ts-node package and do the transpilation of the TS config file at runtime without the user having to use a command that starts with npx cross-env NODE_OPTIONS="--loader ts-node/esm --no-warnings" ?

Meaning we could just do: monocart merge ./shards/**/monocart-report.json -c .\playwright.monocart-reporter-merge.ts and it works?

@cenfun
Copy link
Owner

cenfun commented Sep 20, 2024

Cannot use import statement outside a module

try updating package.json and adding "type": "module"

{
"type": "module"
}

@cenfun
Copy link
Owner

cenfun commented Sep 20, 2024

Meaning we could just do: monocart merge ./shards/**/monocart-report.json -c .\playwright.monocart-reporter-merge.ts and it works?

Yes, it could be a bit trouble to directly load ts files, however, I personally do not want to depend on ts-node just for loading a ts config.
In fact, as a veteran front-end developer, we can do very well without TypeScript.
But there is also a good news is, Node.js will native support TypeScript. see https://nodejs.org/docs/latest/api/typescript.html#type-stripping

Try it with Node.js v22.6.0 +

@edumserrano
Copy link
Author

But there is also a good news is, Node.js will native support TypeScript. see https://nodejs.org/docs/latest/api/typescript.html#type-stripping

Try it with Node.js v22.6.0

Using Node 22.9.0 and running the same command I get:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": Cannot require() ES Module Z:\repos\azdo-lexis-nexis\Apps.Radix.Libraries\ngx-module-federation-tools\playwright.monocart-reporter-merge.ts in a cycle.


Yes, it could be a bit trouble to directly load ts files, however, I personally do not want to depend on ts-node just for loading a ts config.

That is fair, perhaps only worth it if you have significant number of people asking for such a feature.

In fact, as a veteran front-end developer, we can do very well without TypeScript.

I don't want to side track this issue with discussion on whether or not types are useful. I would say that this library is meant to be used with Playwright and Playwright encourages/defaults to using Typescript. This means that users can get into the same situation as me where they have Typescript code they want to share between the Playwright config and the merge CLI config. In this case, making it easier to consume a TS file from the merge CLI makes sense I think.

@cenfun
Copy link
Owner

cenfun commented Sep 20, 2024

I found a new lib called tsimp https://github.com/tapjs/tsimp
It seems simple, I will try it

@cenfun
Copy link
Owner

cenfun commented Sep 21, 2024

Please try [email protected]

  • npm i amaro amaro is a TypeScript parser (It's currently used as an internal in Node.js for Type Stripping)
  • monocart merge ./docs/shard*/index.json Using default config mr.config.ts

BTW, There is a open issue in tsimp which led me to give it up and then find amaro

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

Tried version 2.8.2. The good news is that now it's easy to use a TS config with the merge CLI. It just works. However, I'm having an issue for my case where I want to have some shared code.

The issue is that when I import something from another file it errors.

If i have an import like this:

import { getMonocartReporterOptions } from "playwright.monocart-reporter-options";

It fails with:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": Cannot find package 'playwright.monocart-reporter-options' imported from Z:\repos\azdo-lexis-nexis\Apps.Radix.Libraries\ngx-module-federation-tools\playwright.monocart-reporter-merge.ts

If I add a ./ at the start of import:

import { getMonocartReporterOptions } from "./playwright.monocart-reporter-options";

It fails with a different error:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": Cannot find module 'Z:\repos\azdo-lexis-nexis\Apps.Radix.Libraries\ngx-module-federation-tools\playwright.monocart-reporter-options' imported from Z:\repos\azdo-lexis-nexis\Apps.Radix.Libraries\ngx-module-federation-tools\playwright.monocart-reporter-merge.ts

Not sure if that's an issue with the current implementation in 2.8.2 or if I'm doing something wrong. Can you get this case to work or is it just something in my project?

@cenfun
Copy link
Owner

cenfun commented Sep 21, 2024

The "playwright.monocart-reporter-options" is a file?
can you try "./playwright.monocart-reporter-options.ts"?

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

The "playwright.monocart-reporter-options" is a file?

Yes.


can you try "./playwright.monocart-reporter-options.ts"?

If I try to add the extension I get this error:

An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.

And then if I add the allowImportingTsExtensions to my project it asks for:

Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set

I don't think I can add any of those to my project without creating other problems but just to test this I did it and after I got a different error when running the merge CLI command. I got:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": The requested module 'monocart-reporter' does not provide an export named 'CoverageReportOptions'

The above error is because at the start of the file being imported, playwright.monocart-reporter-options.ts, I have an import like:

import {
  CoverageReportOptions,
  MonocartReporterOptions,
  ReportDescription,
} from "monocart-reporter";

Also, if I don't do any of the above changes and just have a merge config TS file that looks like:

// This is a merge CLI TS file
import {
  CoverageReportOptions,
  MonocartReporterOptions,
  ReportDescription,
} from "monocart-reporter";

I get the same error:

[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": The requested module 'monocart-reporter' does not provide an export named 'CoverageReportOptions'

Lastly, I want to add that these imports, both the one from "playwright.monocart-reporter-options" and the ones from "monocart-reporter", are fine because they do work when running the playwright tests which uses these TS files that we are having issues with on the merge CLI.

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

And I guess you already know this but I wanted to add anyway that if we don't install amaro we get a good error message which makes it easier for the user to know how to fix the issue.

With a TS merge CLI config like this:

// This file is named playwright.monocart-reporter-merge.ts

import path from "node:path";

const reportersOutputDir = path.resolve("./test-reporters/playwright");
const monocartReporterOutputDir = path.resolve(reportersOutputDir, "monocart-reporter");
const options = {
  name: "Playground app for @ln/ngx-module-federation-tools",
  outputFile: path.resolve(monocartReporterOutputDir, "monocart-report.html"),
}

export default options;

And without amaro, when I run:

npx monocart merge ./shards/**/monocart-report.json -c .\playwright.monocart-reporter-merge.ts

I get this error:

The "amaro" module is required for loading ".ts" file, please run "npm i amaro"
[MR] ERROR: failed to load config ".\playwright.monocart-reporter-merge.ts": Cannot destructure property 'transformSync' of '(intermediate value)' as it is undefined.

Then after installing amaro it works as expected 👏

It feels it's almost there, there's just some issue with imports going on...

@cenfun
Copy link
Owner

cenfun commented Sep 21, 2024

CoverageReportOptions is a type not a module or API, so we should import it with type reserved

import type { CoverageReportOptions } from "monocart-reporter";

Can you try above?

TypeScript has very complex configurations and specifications, which can easily lead to various issues. I am not very familiar with TypeScript, and of course, I have not do exhaustive testing. I hope this can fix it; otherwise, I think it should be go back to using ts-node.

@edumserrano
Copy link
Author

CoverageReportOptions is a type, so we should import it with type reserved

Although this is something I've seen some linters recommending at times, my understanding it's that it is an optimization, not a "correct usage" requirement. Meaning that in all my TS projects I have never used import type syntax even when importing types. Anyways, I tried it and it did work. It solved the issue with merge CLI+amaro+TS

So to recap, what I had to do was:

  1. Install amaro: npm i -D amaro
  2. Change the import of files on the merge CLI file from import { getMonocartReporterOptions } from "playwright.monocart-reporter-options; to use relative imports with the file extension such as import { getMonocartReporterOptions } from "./playwright.monocart-reporter-options.ts";
  3. Update the tsconfig.json file to include allowImportingTsExtensions which then says allowImportingTsExtensions requires either noEmit or emitDeclarationOnly. I added the noEmit.

Then given the following contents of a TS merge CLI file:

// This is the playwright.monocart-reporter-merge.ts file

import { getMonocartReporterOptions } from "./playwright.monocart-reporter-options.ts";
import { reportersOutputDir } from "./playwright.shared-vars.ts";

const isShardedTestRun = false;
const options = getMonocartReporterOptions(reportersOutputDir, isShardedTestRun);
export default options;

And running:

npx monocart merge ./shards/**/monocart-report.json -c .\playwright.monocart-reporter-merge.ts

It worked as expected 👏

HOWEVER, the allowImportingTsExtensions broke everything else in my project. 😭

Can't run my Angular app because it errors with:

An unhandled exception occurred: tsconfig.json:32:5 - error TS5023: Unknown compiler option 'allowImportingTsExtensions'.

Can't run jest tests because it errors with:

Error: Jest: Failed to parse the TypeScript config file Z:\repos\azdo-lexis-nexis\Apps.Radix.Libraries\ngx-module-federation-tools\jest.config.ts
TSError: ⨯ Unable to compile TypeScript:
error TS5023: Unknown compiler option 'allowImportingTsExtensions'.

Can't run playground tests because it errors with:

An unhandled exception occurred: tsconfig.json:32:5 - error TS5023: Unknown compiler option 'allowImportingTsExtensions'.

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

Given my previous comment, I don't think the current implementation with amaro makes it easier for a consumer to use a TS file for the merge CLI. I don't even know if I can get rid of the above errors by having a custom tsconfig.json file JUST for the merge CLI where I would apply the allowImportingTsExtensions so that it didn't affect the rest of my project.

However, even if I did that and for instance created a tsconfig.mcr.json I don't know how I would tell the merge CLI to use that tsconfig.mcr.json.

When you said:

I hope this can fix it; otherwise, I think it should be go back to using ts-node.

I'm thinking it might be worth to give ts-node a try OR if you think this is too much hassle to support perhaps we just close this issue and say that there's "partial" support for TS config files and that it's recommended to use any other format for the merge CLI. Up to you @cenfun .

@cenfun
Copy link
Owner

cenfun commented Sep 21, 2024

Perhaps amaro can work well, it's just that we haven't used the correct configuration.
But for me, I don't have your test case to test it. Can you help to try different options?

  • First, find and open file node_modules/monocart-reporter/lib/hooks.js
  • Try some options here
const { code } = transformSync(source.toString(), {
            mode: 'strip-only'
            // other options
        });

For example, try mode: 'transform', see more options here
https://github.com/nodejs/amaro/blob/main/lib/wasm.d.ts

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

I tried that and it didn't work. I also tried other options without any luck 😔

I think the only way we can make progress here is if I give you something you can check for yourself so I took the time and created a minimalistic reproduction of the problem at monocart-merge-cli-demo.

I think the README has all the information you need to be able to reproduce the error but let me know if something doesn't work as expected. The repo will let you easily create sharded test reports using monocart-reporter and then test the merge-cli with a TS config file.

Note

In that demo repo I have used Typescript version 4.3.5 which is what I have on the projects where I'm testing the merge-cli but if you need to update that Typescript version to test things with Amaro then feel free to do so since Amaro as a note on their README saying: The supported TypeScript version is 5.5.4..

Hope this helps 🤞

@edumserrano
Copy link
Author

edumserrano commented Sep 21, 2024

@cenfun Although I'd really like to have an easy way to use a TS file for the merge CLI I want to stress out that it's a minor inconvenient that I can't. If implementing support for this using amaro or ts-node or whatever feels like it's not worth it then so be it and you can close this issue.

You already provide an amazing support for this library 🥇 and I just want you to know that I appreciate that a lot and I don't want to make you spend time in quality of life improvements if you don't think they're worth it.

@cenfun
Copy link
Owner

cenfun commented Sep 22, 2024

Thanks for the minimalistic reproduction repo which help to fix issue easily.
The new patch is [email protected], please have a try refer to my testing following.

TypeScript background in your project

  • Version is 4.3.5 with your own tsconfig.json options
  • Parsed by Playwright which is powerful and full featrue supported
  • Resolved ts file path without extensions, even no ./ or ../ prefix

So the merge CLI need to do same thing like Playwright do. Obviously, it's not that easy, that's why simply using amaro doesn't work.
The new patch is support new CLI option --import to load the custom TypeScript loader

monocart merge --import my-ts-loader

It requires Nodejs 18.19.0 +

Testing of popular TypeScript loaders

  • tsx - ✅passed
monocart merge ./shards/**/monocart-report.json --import tsx

The only passed lib for your project. This is because it's powerful ts file resolver

monocart merge ./shards/**/monocart-report.json --import ts-node
# or
monocart merge ./shards/**/monocart-report.json --import ts-node/esm
monocart merge ./shards/**/monocart-report.json --import ts-node/register

I didn't make it, and found there is a issue in Node.js 20: TypeStrong/ts-node#1997
Maybe it works in Nodejs 18 but I haven't test it.

monocart merge ./shards/**/monocart-report.json --import tsimp/import

It seems that the ts file path can not be resolved well. Maybe there are some options need to be set in tsconfig.json I am no idea.

monocart merge ./shards/**/monocart-report.json --import amaro/register

The amaro/register currenly is working with Nodejs 22 --experimental-strip-types, so it doesn't work either (My local Nodejs is v20). and I don't think it can resolve the ts file path well.

@edumserrano
Copy link
Author

[email protected] works like a charm. I just had to install tsx with npm i -D tsx and then I ran npx monocart merge --import tsx ./test-reporters/shards/**/monocart-report.json -c ./playwright.monocart-reporter-merge.ts and everything worked as expected.

Thank you very much for your patience and getting this done @cenfun 🤗

I'm closing this issue but I would recommend that you add some note to the README on the Using merge CLI section to tell users how they can use TS files for config with this --import option.

@edumserrano
Copy link
Author

@cenfun I couldn't find anything on the README about this feature you've implemented. What do you think about adding something to the README under the Using merge CLI section about how to use Typescript configuration files?

Saying:

  • it requires node 18.19.0+
  • installing tsx: npm i -D tsx
  • using the --import tsx flag

@cenfun
Copy link
Owner

cenfun commented Sep 30, 2024

Thanks, the README is updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants