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

docs: update documentation for using TypeScript configs #3521

Closed
wants to merge 1 commit into from
Closed

docs: update documentation for using TypeScript configs #3521

wants to merge 1 commit into from

Conversation

jonkoops
Copy link

The existing documentation was outdated, it is possible to simply use a .ts extension for your config file.

@karmarunnerbot
Copy link
Member

Build karma 289 completed (commit d073839c2d by @jonkoops)

@karmarunnerbot
Copy link
Member

Build karma 288 completed (commit d073839c2d by @jonkoops)

@karmarunnerbot
Copy link
Member

Build karma 290 completed (commit 5a58b2a8cd by @jonkoops)

@karmarunnerbot
Copy link
Member

Build karma 289 completed (commit ae5fb60400 by @jonkoops)

@jonkoops jonkoops changed the title Update documentation for using TypeScript configs chore: update documentation for using TypeScript configs May 19, 2020
@karmarunnerbot
Copy link
Member

Build karma 291 completed (commit 05aa835c0f by @jonkoops)

@karmarunnerbot
Copy link
Member

Build karma 290 completed (commit 05aa835c0f by @jonkoops)

@AppVeyorBot
Copy link

Build karma 2687 completed (commit d073839c2d by @jonkoops)

@AppVeyorBot
Copy link

Build karma 2688 completed (commit ae5fb60400 by @jonkoops)

@AppVeyorBot
Copy link

Build karma 2689 completed (commit 05aa835c0f by @jonkoops)

@devoto13
Copy link
Collaborator

devoto13 commented May 19, 2020

Do you have an example with tsconfig.json with module set to esnext, where this will just work on Node 10 (latest Node supported by Karma)? I'm pretty sure it still fails and this section explains how to overcome the limitation.

Ref: #3274

@jonkoops jonkoops changed the title chore: update documentation for using TypeScript configs docs: update documentation for using TypeScript configs May 19, 2020
The existing documentation was outdated, it is possible to simply use a `.ts` extension for your config file.
@karmarunnerbot
Copy link
Member

Build karma 292 completed (commit 4596bef53a by @jonkoops)

@jonkoops
Copy link
Author

jonkoops commented May 19, 2020

@devoto13 I've tested this on Node 10 and Node 12 locally both seem to work. In this case I have a specific tsconfig.json with module set to commonjs.

To be specific this is how we're currently running this code in our project from our NPM scripts:

TS_NODE_PROJECT=tsconfig.karma.json karma start

Why would the tsconfig.json need to be set with module to esnext?

@karmarunnerbot
Copy link
Member

Build karma 291 completed (commit 4596bef53a by @jonkoops)

@devoto13
Copy link
Collaborator

devoto13 commented May 19, 2020

In this case I have a specific tsconfig.json with module set to commonjs.

This is the key difference here. Most of the frontend projects use module set to esnext to improve tree shaking in the default tsconfig.json. One of the big ones is Angular CLI.

Your approach is also a valid way to do it, but it will not work on Windows for example and still requires an extra file. So what is the benefit?

@devoto13
Copy link
Collaborator

IMO the best solution to this would be to deprecated and eventually remove built-in support for different languages and expose -r, --require flag on the CLI similar to how other tools handle this problem.

@AppVeyorBot
Copy link

Build karma 2690 completed (commit 4596bef53a by @jonkoops)

@jonkoops
Copy link
Author

In my case I got some errors from including ts-node directly as I could not get any options passed in to work as intended (some missing typedefs for modules so I wanted to pass --transpileOnly).

Of course the tsconfig.json that is included from the project could be used, I found it more logical to include a seperate config for Karma only. Matter of preference I guess.

Since we're going to need to use CommonJS for modules for the forseeable future why not override the module option when initializing Karma so that the result will always be in CommonJS?

This could also be done from the side of the user by passing in compiler options to override:

TS_NODE_COMPILER_OPTIONS={ "module": "commonjs" } karma start

@devoto13
Copy link
Collaborator

devoto13 commented May 19, 2020

Unfortunately TS_NODE_COMPILER_OPTIONS={ "module": "commonjs" } karma start will not work on Windows.

I'm ok updating the documented workaround if you can show a concrete use case, which is impossible with the currently documented approach, but in general we should focus on fixing the root cause instead of improving the workaround for it.

You should be able to achieve transplie-only by using ts-node/register/transpile-only instead of ts-node/register according to their README.

UPD Or rather passing it to the register() - https://github.com/TypeStrong/ts-node/blob/master/src/index.ts#L115.

@jonkoops
Copy link
Author

Unfortunately TS_NODE_COMPILER_OPTIONS={ "module": "commonjs" } karma start will not work on Windows.

Not even using a tool like cross-env? I mean this is a known restriction for Windows.

I'm ok updating the documented workaround if you can show a concrete use case

The only use-case that I have here is to avoid using additional files for Karma configuration. We can close this PR if it creates more issues than it solves, although I think it's good for users to be aware of the option.

On a side-note I think #3274 actually describes the problem I was running into passing other configuration options.

But in general we should focus on fixing the root cause instead of improving the workaround for it.

What is the root cause issue here? I see some people having issues with Karma including ts-node by accident as it's dependency of some other code. Next to that it also seems that some other people (myself included) were having problems passing in custom options.

@devoto13
Copy link
Collaborator

devoto13 commented May 19, 2020

Not even using a tool like cross-env? I mean this is a known restriction for Windows.

Again, yes you can solve it. But the idea was to make configuration easier and adding another dependency does not sounds like making things easier.

The only use-case that I have here is to avoid using additional files for Karma configuration. We can close this PR if it creates more issues than it solves, although I think it's good for users to be aware of the option.

It should be fine to add this as an alternative approach to configuration customisation section. Something along these lines, alternatively you can customise ts-node behaviour using environment variables (link to the relevant ts-node docs), here is an example. But I don't think we should replace the current docs as env vars approach also has caveats.

What is the root cause issue here? I see some people having issues with Karma including ts-node by accident as it's dependency of some other code. Next to that it also seems that some other people (myself included) were having problems passing in custom options.

Basically Karma automatically runs require('ts-node').register() without parameters if it is a project dependency (see here). Also #3521 (comment). What I think is a better approach is to deprecate and eventually remove this implicit behaviour from Karma. And instead expose a command line flag, so users can configure whatever preprocessor they like with whatever options they like and Karma does not need to care about it.

E.g.

$ karma -r ts-node/register start karma.conf.ts
$ karma -r ./my-custom-register-with-options.js start karma.conf.ts
$ TS_PROJECt=tsconfig.karma.json karma -r ts-node/register start karma.conf.ts

So basically preprocessing of the config is users responsibility and Karma does not have any built-in logic there.

Alternative approach was to do something like #2186, but IMO this will complicate current situation even further.

@devoto13
Copy link
Collaborator

The above is just an idea, this should be opened as an issue and discussed before starting the implementation.

@jonkoops
Copy link
Author

Ok, it's clear to me now. I'll go ahead and close this PR for now since it doesn't add much and once the proposed changes are made to load a custom registration it should be resolved anyways.

Thanks for taking the time to explain the issue @devoto13.

@jonkoops jonkoops closed this May 20, 2020
@jonkoops jonkoops deleted the patch-1 branch May 20, 2020 11:11
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

Successfully merging this pull request may close these issues.

4 participants