regression of Unexpected token import when using es2015 modules

[piotrwitek]

• Issue
  regression of previously resolved issue Not processing correctly when using es2015 modules #121 Hardcode module commonjs #123

SyntaxError: Unexpected token import when using es2015 modules with following config:

//tsconfig
{
  "compilerOptions": {
    "target": "es5",
    "module": "es2015",
    "moduleResolution": "node",
    ...
  }
}

// jest
{
  "moduleFileExtensions": [
    "ts",
    "tsx",
    "js"
  ],
  "transform": {
    ".(ts|tsx)": "<rootDir>/node_modules/ts-jest/preprocessor.js"
  },
  "globals": {
    "__TS_CONFIG__": "<rootDir>/src/tsconfig.json"
  }
}

• Fix
  If following line is taken out of enclosing if statement, problem is fixed.

d26dbf6#diff-e1495d267619047a7cca5cfe8f692729R88

• Expected behavior

should set config.module = 'commonjs'; when loading external tsconfig with "module": "es2015"

• Actions
  • add a new test case to prevent further regressions of this case: https://github.com/kulshekhar/ts-jest/blob/master/tests/__tests__/tsconfig-default.spec.ts#L87

[kulshekhar]

@piotrwitek see #146

There was a comment that if users don't care about coverage but want to set module to a different value, they should be allowed to do so.

So here are the options:

• If you care about coverage, do not set module to anything other than commonjs in jest > globals > __TS_CONFIG__
• If you don't care about coverage and want to set module to another value during testing, you'll need to set it in jest > globals > __TS_CONFIG__ (either directly or via an included config file).

Note that if the config file is anything but 'tsconfig.json' in the root directory, it will leave the module property untouched.

[kulshekhar]

@piotrwitek I should also add that the tsconfig file for testing can be different from your project's main tsconfig.json. So you can set module to es2015 for your project and to commonjs for testing

[piotrwitek]

@kulshekhar Thanks for your answer, this change has broken my build when updated from 19.0.1 to 19.0.8, which violate semver :/

I still think that current solution is not optimal and good enough for my use case, I think a new option could be introduced:

__TS_CONFIG__: {"module": "commonjs"}
__TS_CONFIG_PATH_: "<rootDir>/src/tsconfig.json"

This solution could be flexible enough and if implemented without changing old behaviour for __TS_CONFIG__, should be backward compatible

e.g. if only __TS_CONFIG__ is used behave as old one, if both are provided use new behaviour.

Then on next major jump deprecate the old way and promote new configuration.

Is this something you would like to consider @kulshekhar ?
I could contribute work for this change.

[kulshekhar]

This project doesn't follow semver, sorry.

The change you suggest would break almost every other setup that uses an external config file. Can you share the tsconfig that you're using for testing? In most cases, you don't even need one

[piotrwitek]

@kulshekhar could you explain why you think it will break other setup?
because as I stated above:

• if only TS_CONFIG is provided behave as currently
• if both TS_CONFIG & TS_CONFIG_PATH are provided use new behaviour

IMO this doesn'e break anything, but maybe I don't understand something

[piotrwitek]

in detail I would add a new if branch checking if (TS_CONFIG & TS_CONFIG_PATH) are provided and only then execute a new logic, make sense?

[kulshekhar]

Currently TS_CONFIG can be either an object or a filename. Moreover, I don't like the idea of introducing more global variables - it can complicate matters unnecessarily.

Again, can you share the tsconfig that you're using for testing

[kulshekhar]

please also share the jest section from package.json

[piotrwitek]

{
  "compilerOptions": {
    "allowJs": false,
    "declaration": false,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "forceConsistentCasingInFileNames": true,
    "importHelpers": true,
    "jsx": "react",
    "lib": [
      "dom",
      "es2016",
      "es2017.object"
    ],
    "target": "es5",
    "module": "es2015",
    "moduleResolution": "node",
    "noEmit": true,
    "noEmitHelpers": true,
    "noEmitOnError": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitAny": true,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "outDir": "../out/src/",
    "pretty": true,
    "removeComments": true,
    "sourceMap": true,
    "strictNullChecks": true
  },
  "include": [
    "**/*"
  ],
  "exclude": [
    "node_modules"
  ]
}

{
  "automock": false,
  "moduleFileExtensions": [
    "ts",
    "tsx",
    "js"
  ],
  "transform": {
    ".(ts|tsx)": "<rootDir>/node_modules/ts-jest/preprocessor.js"
  },
  "testRegex": "(/__tests__/.*|\\.(test|spec))\\.(ts|tsx|js)$",
  "notify": false,
  "verbose": true,
  "globals": {
    "__TS_CONFIG__": "<rootDir>/src/tsconfig.json"
  }
}

[kulshekhar]

Have you tried removing the __TS_CONFIG__ section from jest > globals?

[kulshekhar]

In the worst case - suppose you need all these settings for testing as well - you could create a separate file for testing, like so:

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "module": "commonjs"
  }
}

and use this new file in jest > globals > __TS_CONFIG__

[piotrwitek]

@kulshekhar yes I know but this is a workaround not a solution
to be honest I think current behaviour is confusing because it can be either an object or a filename, having it separated would make it more clear

for example ts-node & ts-loader and some other libraries I had used provide a way to load a tsconfig from a custom path and override compilerOptions on top of that, I don't see how it could complicate anything

[kulshekhar]

@piotrwitek I agree this is confusing. But I accepted this as a PR a while back and didn't give it much thought then. I didn't think it this project would be used by more than a few folks!

I have plans to refactor the whole project to remove cruft and make it easier to extend. Unfortunately I'm not sure when I'll be able to carve out some time.

For now, I'm trying to help users of this package by fixing bugs and issues that are show stoppers. Would the workaround suggested above be an acceptable solution for you for the time being?

[piotrwitek]

@kulshekhar I understand, this is not an easy work, I really appreciate your work don't get me wrong, just wanted to suggest a way to improve it somehow, maybe it's not the best solution but might be some direction to follow :)

Suggested workaround is the best solution possible right now and should be fine for now, and I'm still open to help in the future.

[alexjoverm]

@kulshekhar count on me if you need help for that refactor, you're not alone ;) I'd like to help if I can

[kulshekhar]

@alexjoverm Thanks! I'm waiting for the next jest version to land but in the meanwhile, we can start by creating a checklist of what needs to be done

[alexjoverm]

@kulshekhar some things that roughly come up to my mind:

• Identifying what are the current requirements and remove code meant for initial obsolete requirements (for instance, how can we expect the tsconfig? as a file?)
• Centralise interfaces and constants to avoid hardcoding
• Breaking up long functions into smaller chunks with more context
• Updating the tests accordingly

I'm saying this by having taken a peak to the code, but I have no actual context :). Where can we create that checklist? What are your suggestions @kulshekhar ?