ts-node REPL treats unparenthesized object literals as block scopes, doesnt align with node REPL behavior

[jhmaster2000]

Search Terms

repl object literal block scope

Expected Behavior

Input string: { key: "value" }
Node REPL behavior, also expected from ts-node:

Actual Behavior

Same input string, how ts-node REPL behaves:

Steps to reproduce the problem

In a terminal with ts-node installed:

1. Run ts-node
2. Enter { key: "value" }
3. Notice how {}'s were treated as a block scope, key as a (unused) label, leaving "value" as the resulting string literal of the expression, instead of the expected object as seen in node REPL above

Minimal reproduction

N/A (See above)

Specifications

• ts-node version: v10.7.10
• node version: v16.14.0
• TypeScript version: v4.6.2
• tsconfig.json, if you're using one: N/A (REPL)
• Operating system and version: Android 11 aarch64 (Termux 0.118.0)

[cspotcode]

To debug this you'll have to check where we're transforming REPL inputs from TS to JS and inspect the output. You can see in repl.ts or repl.js where we call the compiler, then use a diffing library to identify the parts that came from your most recent input. Take a look at the output from the TS compiler.

[jhmaster2000]

@cspotcode hey there, I am taking a stab at this, however I have encountered a roadblock with tests, they seem to be entirely broken on ts-node's current master branch? Is this correct or am I doing something wrong somehow?

The above is the result of literally just git clone https://github.com/TypeStrong/ts-node.git, then running npm install followed by npm run test-local, a fresh clean copy of the master branch without any of my changes, just to make sure it wasn't something I wrote in the code breaking it indirectly somehow... My only thought is maybe tests only work on a specific Node version? But if so, which one, and is it documented anywhere...?

It errors on this line, which doesn't really make much sense to me:

ts-node/ava.config.cjs

Line 35 in 98f30e8

expect(() => {createRequire(resolve(__dirname, 'tests/foo.js')).resolve('ts-node')}).toThrow();

[jhmaster2000]

Update
I have found the cause of the issue above, it's caused by having NODE_PATH environment variable set, containing a path where ts-node is installed. The fix is simple, to just blank out NODE_PATH before running ava, however this adds a new dev dependency, cross-env, is this acceptable? Not sure on the policies on adding new dependencies for this repository.

[cspotcode]

Our tests run nightly on a range of node versions and typescript versions, both Linux and Windows. Check GitHub Actions to see if they are passing and to see the test configuration. Read CONTRIBUTING.md for more information.

You should be able to manipulate environment variables in our ava config file: ava.config.cjs

[jhmaster2000]

I did notice the environmentVariables option in ava.config.cjs, unfortunately that doesn't work, as by the time those are applied, node already parsed NODE_PATH and cached it internally, so changing it at runtime is useless, hence why I had to blank it out prior to running ava using cross-env.

[cspotcode]

I'm hesitant to add this bit of complexity since, as far as I know, no one else has hit this problem. You can choose to unset NODE_PATH in whatever way is most convenient for you to write tests. And if/when you submit a pull request, it will run correctly on Github Actions because Github Actions does not set that environment variable either.

However, we can add a warning to ava.config.cjs that detects if NODE_PATH has been set and instructs the developer to unset it prior to any development work. The warning can include a link to this thread, too.

[jhmaster2000]

There isn't really any added complexity on the user-side if cross-env is simply added to the test-local script, you will still just run npm run test-local as normal, and you can even still run ava directly if you don't have NODE_PATH set, it will also work as usual, it's pretty much what you suggest, but it automatically corrects it for tests rather than asking the user to unset it on their end, which can get annoying fast, myself for example, I have other projects on my machine which rely on NODE_PATH to work correctly, so just ditching it isn't viable, and remembering to unset it everytime to run tests will get bothersome rather than just have it nicely automatically clear the env var. I do however understand and am aware I appear to be an edge case indeed, so I can see where you are coming from with not wanting to add an extra dev dependency just because 1 person had an issue, but I wanted to further clarify how there's no real added complexity to the development process as I felt like that may have been unclear a bit? If you really don't want to do it I understand and can do your suggestion of a warning instead, it is what I had originally gone for before realizing it was fixable by clearing the variable anyway.

[cspotcode]

That's a fair point, but I'd rather go with the warning. If it's easier for you, feel free to add cross-env to your pull request, and I'll remove it before merging. I don't mind doing that.