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

fix: set correct module format for TypeScript #3139

Closed
wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Sep 18, 2018

When having a root tsconfig which is used by the application with module value other than commonjs it would fail if you have an import statement in the config ts file with an error SyntaxError: Unexpected token This is due that under node module target should be set for commonjs

Relates to angular/angular-cli#12294

When having a root tsconfig which is used by the application with module value other than `commonjs` it would fail if you have an `import` statement with an error `SyntaxError: Unexpected token` This is due that under node module target should be set for `commonjs`
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Can you write a test that fails without this change and passes with it?

@alan-agius4
Copy link
Contributor Author

@johnjbarton I'd happily do that but where should it be placed? Also this would imply of adding ts-node and typescript as a devDependency is that ok?

@johnjbarton
Copy link
Contributor

No, I'm not keen on more deps for this small feature.

Why isn't the default commonjs? If we set commonjs who do we break?

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Sep 19, 2018

commonjs is the default module format for TypeScript, however, if a user of karma create a tsconfig in their project with a different module for instance es2015, which is pretty common if you are developing a web app this breaks karma as es2015 module won't be able to run on node. This is due that ts-node will will try to look for a tsconfig.json inside the user project and transpile the file using those configurations.

I have create a small repo illustrating this;

If you remove this https://github.com/alan-agius4/karma-config-ts-issue/blob/master/tsconfig.json#L3 karma won't throw an error. However in most cases if you are developing a web app you'd want this module format.

Changing this to commonjs from karma side shouldn't break anyone.

@johnjbarton
Copy link
Contributor

Thanks for the added information. I understand that your approach will work for many users. However, I wonder if we can find a different solution to the original problem. Any values we put in the code to override tsconfig have the potential for breaking someone or preventing someone from using TypeScript on karma configs.

Couldn't we update the docs to explain how to use TypeScript configs without the ts-node hack in karma? If a user writes a file ./karma.conf.js which contains:

require('ts-node').register({ compilerOptions: { module: 'commonjs' } });
require('./karma.conf.ts');

then the original problem is solved and users can simply customize the solution for other cases without waiting for upstream changes.

@alan-agius4
Copy link
Contributor Author

@johnjbarton, I can certainly do that. Where would you see this doc to fit?

@johnjbarton
Copy link
Contributor

Near the top of https://github.com/karma-runner/karma/blob/master/docs/config/01-configuration-file.md

To balance the goal of clarity and users noticing two options for loading TS configs, consider a short sentence near the top pointing to the customizable solution and then the customizable solution at the end of the Overview.

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Sep 22, 2018

super-seeded by #3140

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.

2 participants