Skip to content
This repository was archived by the owner on Oct 14, 2020. It is now read-only.

Conversation

@OverZealous
Copy link
Contributor

  • Now the languageVersion is not passed into the Karma/Node runner code, so we don't get Node version errors.
  • Updated Node docs to mention Karma testing on TS

- Now the `languageVersion` is not passed into the Karma/Node runner code, so we don't get Node version errors.
- Updated Node docs to mention Karma testing on TS
@OverZealous OverZealous requested a review from kazk September 5, 2017 15:09
Copy link
Member

@kazk kazk left a comment

Choose a reason for hiding this comment

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

Looks good to me

runKarma(opts, runCode, fail, interfaceType, config);
const nodeOpts = Object.assign({}, opts);
// run on the default node version
delete nodeOpts.languageVersion;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I was curious how long ago Node 2.4 was and apparently it doesn't even exist :D (it was io.js 2.4 released 2015-07-17).

If specifying the target for Karma is useful, we can do that too. I didn't touch Karma's configuration in #476 because it was explicitly set to ES5.
https://github.com/Codewars/codewars-runner-cli/blob/aeb1d5d7628000217dc79b8a3d0594f9668d4f5d/lib/runners/typescript.js#L142-L147

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I definitely thought about it. I couldn't imagine a good scenario for setting the Karma ES target. Nobody building websites today can safely target anything newer than ES5, and ES3 is too restrictive for most modern JS.

I'll leave it for something we can add in later if it become useful. As it is, we only support a single version of TS, and I'm not 100% sure how significantly the different target versions change the output in this case.

@jhoffner jhoffner merged commit 1c71c58 into codewars:master Sep 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants