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

Add --ts flag to properly support projects with mixed .js and .ts files. #550

Closed
wants to merge 1 commit into from

Conversation

kyegupov
Copy link

@kyegupov kyegupov commented May 3, 2019

Properly addresses issues #549 and #526

Copy link
Member

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

I'm curious why we can't just detect the double-instrumentation and prevent it? The problem seems like it arises when you have something like this:

// load-ts.js
require('./ok.ts')
// ok.ts
import * as t from './'
t.pass('this is fine')
node -r ts-node/register -r ts-node/register load-ts.js

But when I run that, it seems like it works fine, so I'm a little bit confused about whether I understand the problem this is trying to address. Do you have an example where I can reproduce the problem you're experiencing?

Also: note that this PR is to the master branch, which is version 13. To address the issue in a v12 legacy release, it'll have to be done (a little bit differently) in the v12 branch. Maybe not an issue you care about if you want to upgrade anyway, but worth mentioning.

@@ -544,11 +544,15 @@ Much more documentation available at: https://www.node-tap.org/
}),

esm: flag({
default: process.env.TAP_NO_ESM !== '1',
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated?

@isaacs
Copy link
Member

isaacs commented May 4, 2019

Oh! Maybe what you're describing only happens when there are two different instances of ts-node in play? I copied ts-node to another folder, and ran it this way. This is an interesting problem:

$ node -r ts-node-duplicate/register -r ts-node/register load-ts.js

/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:240
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
ok.ts:2:23 - error TS2304: Cannot find name 'exports'.

2 Object.defineProperty(exports, "__esModule", { value: true });
                        ~~~~~~~
ok.ts:3:9 - error TS2580: Cannot find name 'require'. Do you need to install type definitions for node? Try `npm i @types/node`.

3 var t = require("./");
          ~~~~~~~

    at createTSError (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:240:12)
    at reportTSError (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:244:19)
    at getOutput (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:360:34)
    at Object.compile (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:393:11)
    at Module.m._compile (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:439:43)
    at Module.m._compile (/Users/isaacs/dev/js/tap/node_modules/ts-node-duplicate/src/index.ts:439:23)
    at Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at require.extensions.<computed> (/Users/isaacs/dev/js/tap/node_modules/ts-node-duplicate/src/index.ts:442:12)
    at Object.require.extensions.<computed> [as .ts] (/Users/isaacs/dev/js/tap/node_modules/ts-node/src/index.ts:442:12)
    at Module.load (internal/modules/cjs/loader.js:628:32)

@isaacs
Copy link
Member

isaacs commented May 4, 2019

Interestingly, if I do what the error message says, and npm i @types/node, it fixes the problem in that case as well. Of course, not as efficient to double-transpile it since the second one has nothing to do, but still, not a crash.

@isaacs
Copy link
Member

isaacs commented May 4, 2019

It seems like a cleaner way to address this would be to treat --ts or --no-ts in the same way as the --esm flag. Ie, it's a flag that just means "do -r ts-node/register for any /\.tsx?$/ files". If you want to provide your own mechanism, you'd do tap --test-arg=--require=my-ts-node/register --no-ts test/foo.ts. To make it even easier, you could put this in your package.json:

{
  "tap": {
    "ts": false,
    "test-arg": [
      "-r",
      "my-ts-node/register"
    ]
  }
}

Would that address the problem?

isaacs added a commit that referenced this pull request May 6, 2019
This allows users to do `tap --no-ts --node-arg=--require=my-ts-loader`
to specify their own TypeScript loader that might conflict with tap's
built-in version.

Fix #550
Fix #549
@isaacs
Copy link
Member

isaacs commented May 6, 2019

@kyegupov Can you check out 9fd8a12 and see if that would give you what you need?

isaacs added a commit that referenced this pull request May 6, 2019
This allows users to do `tap --no-ts --node-arg=--require=my-ts-loader`
to specify their own TypeScript loader that might conflict with tap's
built-in version.

Fix #550
Fix #549
isaacs added a commit that referenced this pull request May 7, 2019
This allows users to do `tap --no-ts --node-arg=--require=my-ts-loader`
to specify their own TypeScript loader that might conflict with tap's
built-in version.

Fix #550
Fix #549
@isaacs isaacs closed this in ecfece6 May 18, 2019
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.

None yet

2 participants