oclif / config

base config object and standard interfaces for oclif components
MIT License
25 stars 36 forks source link

config breaks jest inline snapshots due to ts-node/register #98

Open G-Rath opened 4 years ago

G-Rath commented 4 years ago
import HelloWorld from '@src/commands/hello_world';

describe('fmt', () => {
  it('writes the results to disk', async () => {
    try {
      await HelloWorld.run([]);
    } catch (e) {
      console.log(e);
    }

    expect({}).toMatchInlineSnapshot();
  });
});

The above will fail in a typescript project due to config setting up tsNode.register. This is because Jest sees there's already a transformer for TypeScript, and so doesn't apply its own transformer.

This is "fine", except that the output of ts-node doesn't match the output of Babel (which is what jest uses); when writing inline snapshots, jest looks for all toMatchInlineSnapshot matchers by walking the ast as provided by Babel.

As such, it fails to find the snapshot matcher because while Babel is walking the AST, it didn't do the transformation, so the line numbers are different and it never finds the matcher.

In general, @oclif/config probably shouldn't be using ts-node like this, as it's not really needed: it's encouraging users to ship TS files when they should be compiled, and this behaviour can be replicated in userland by calling tsNode.register in your bin script, or by passing it to node as part of the call (for when testing, which I assume is why this feature was added).

Additionally, it's probably going to cause oclif some pain when ESM modules land, as it'll likely force people to stick on commonjs.

Removing ts-node would solve a number of issues that have been opened around oclif packages.

G-Rath commented 4 years ago

@RasPhilCo @jdxcode I've confirmed that removing ts-node from config while leaving the rest of registerTSNode & surrounding code is painless, with the previous functionality being restorable with the following package.json script:

"cli": "node -r ts-node/register bin/<mycommand>",

Let me know if you're open to a PR

RasPhilCo commented 4 years ago

@G-Rath open to this idea. Initial thoughts: 1) Maybe the registering can be handled in a bin/dev shim. 2) We likely need to still back-support for sometime but oclif generators could start producing a new shim

G-Rath commented 4 years ago

If I understand you correctly, those should be fine - the problem is about that the registering currently exists in oclif instead of userland, so you can't opt-out and have to carry the dependency with you.

So (again if I'm understanding you correctly) having the generator generate a new bin/dev script for typescript projects that does the registering would be fine since I think that's a cool default to give them and it lets the user remove it if they decide they won't want the ts-node dependency in their tree.