Different TS-JEST / TS emitted code when importing 3rd party libs

[shlomiassaf]

Continuing the discussion from issue 209

For the same code compiled with TS, ts-jest emits a different output.

• Both tsconfig file are identical (one for regular TS compilation, one for ts-jest)
• allowSyntheticDefaultImports set to true.

When using this statement:

import * as pathToRegexp from 'path-to-regexp';

In TS compilation without ts-jest pathToRegexp is a function
In TS compilation with ts-jest pathToRegexp is an object.

It's not deterministic, different outcome for the same TS compilation process

[shlomiassaf]

@kulshekhar From what you explained, there is another transpilation happening...

TS -> TS emitted code -> babel -> ES5 JS

Since TypeScript can emit ES5 why would we need babel?

[kulshekhar]

When allowSyntheticDefaultImports is set to true, ts-jest processes the output of the typescript compiler further using babel.

When it is set to false, only typescript is used.

This was done recently in PR #172

That might be a good starting point to understand why this was done.

(I could attempt an explanation but because I don't use babel or allowSyntheticDefaultImports, there's a chance I might miss something or state something incorrectly)

[shlomiassaf]

@kulshekhar I understand the logic behind that PR, yet it solved one issue just to introduce an other.

If I set allowSyntheticDefaultImports to false I get an error from TS.
If I set it to true I can't run tests.

[kulshekhar]

@shlomiassaf there was a short discussion somewhere around whether we should use another setting to tell jest to use babel as well. Would such a setting solve this issue?

edit: here's that conversation

[GeeWee]

It seems to me it'd be a good idea, and then just default to true if allowSyntheticDefaultImports

(Might also be a good idea to at the same time move the TS_CONFIG into a seperate "ts-loader" key - it seems a little strange to keep it in globals. Would break backward compat though)

[marcusjwhelan]

@GeeWee when you say move TS_CONFIG into a separate "ts-loader" what exactly do you mean by this? Are you saying another JSON field in Jest? Or are you saying you would like to have more than one tsconfig file to point to?

Also I agree with the allowSyntheticDefaultImports instead of having to specify it in my tsconfig file. It makes more sense to me that this would need to be in the test config since I do not need it in my tsconfig file.

[GeeWee]

Yeah another JSON field, like:

{
"jest" : {
"ts-loader": {
  "__TS_CONFIG__" : {
   ...
  },
  "transformUsingBabel": true,
}

It seems a little silly having to expose __TS_CONFIG__ as a global variable to all the test code.

[kulshekhar]

While I agree about the TS_CONFIG bit, I'm a bit hesitant to introduce breaking changes.

Adding a flag to control babel usage seems sensible though - with the default value (absence) leaving the behavior unchanged. So if this isn't set, ts-jest will process through babel if allowSyntheticDefaultImports is set to true.

I'm not sure what the best way to name this flag would be though:

• transformUsingBabel (or something on these lines like transformUsingBabelAfterTypescript) - this will have a default value of true to prevent a breaking change. Users will have to set this to false to address the issue we're discussing in this thread. This seems kinda counter-intuitive

• transformUsingTypescriptOnly - in this case, an absence of this flag will be the same as if the flag is set to false, avoiding a breaking change.

[GeeWee]

I totally agree with you about the breaking changes.
I feel however like it's an easy fix to make for the end user - and we can provide them with a deprecation warning, and maintian backwards compatibility for a while if needed?

Another point there is, where do you want the babel flag?
You can't stick it in the __TS_CONFIG_ because that'd make it impossible for someone to use it with an actual .tsconfig file. Another global variable?

I think transformUsingBabel makes more sense.
I would set it to be false by default, unless allowSyntheticDefaultImports is set to true. This makes more sense to me and is not a breaking change.

[kulshekhar]

• we can open a new issue about the breaking change and discuss that separately

• the babel flag can't go in __TS_CONFIG__. It will need to be outside it.

• When allowSyntheticDefaultImports isn't set, currently, we never invoke babel. I'm not sure there's a need for this. So the new flag will be used only when allowSyntheticDefaultImports is true.

• Now if the default behavior uses babel, the only case when this new flag will be used will be if a user wants to prevent the use of babel. In this state, the term transformUsingBabel doesn't quite signify what the user wants to do. Something like transformUsingTypescriptOnly or doNotUseBabel seems more intuitive. That's just my view though.

[GeeWee]

Good idea about the breaking change issue #217

I agree.

Oh so you're thinking to only check the flag if allowSyntheticDefaultImports is true?
I agree it's the only case where it makes sense to use babel, but I feel like a flag that only gets checked if a specific configuration option in .tsconfig is true, is rather confusing to explain.

I get what you're saying, but as an outsider I was really surprised to learn that ts-loader used babel if allowSyntheticDefaultImports was set - in my mind it makes more sense to feel like you're opting-in to babel? It probably doesn't matter much.

[kulshekhar]

Oh so you're thinking to only check the flag if allowSyntheticDefaultImports is true?

Yes, because currently babel is used only when allowSyntheticDefaultImports is true. Come to think of it, ts-jest is currently using allowSyntheticDefaultImports where the new flag should be used. If we introduce this new flag, we can remove the check for allowSyntheticDefaultImports because it'll become irrelevant.

I get what you're saying, but as an outsider I was really surprised to learn that ts-loader used babel if allowSyntheticDefaultImports was set

This wasn't always the case. The PR that did this was merged only a few weeks ago.

in my mind it makes more sense to feel like you're opting-in to babel

If we were implementing this from scratch, I'd agree 100%. As things stand, users using this (that PR fixed issues for a lot of people) are opted in to babel by default. And those who aren't using this don't really care about babel (probably).

Well, we can use transformUsingBabel and replace the check for allowSyntheticDefaultImports with a check for transformUsingBabel. This will be a breaking change for some users but I think it'll remove confusion that this has caused

[GeeWee]

I've mulled it over. There are three options as I see it.

1. A skipBabel flag that is set to false by default, but can be set to true. This is only checked if allowSyntheticDefaultImports is true

Pros.

1. Not a breaking change
2. Easy to explain.
3. A sensible default, I think most people with allowSyntheticDefaultImports set to true want this.

Cons.

1. Makes it impossible to use the babel transform without allowSyntheticDefaultImports

If we can't think of a use case where you would be interested in using babel without allowSyntheticDefaultImports I think this is the best idea.

2. A transformUsingBabel flag that replaces the allowSyntheticDefaultImports as you suggested. This flag would default to false.

Pros.

1. Makes it possible to use babel transform without requiring allowSyntheticDefaultImports to be true.

Cons.

1. Breaking change.

2. I think most people who use allowSyntheticDefault are interested in this functionality, so I feel like false is not a sensible default.

3. A transformUsingBabel flag that behaves the same as the suggestion above, except it defaults to the value of allowSyntheticDefaultImports.

This removes all the cons from the suggestion above, but it is a little harder to explain to the users.

Thoughts?

[kulshekhar]

Another thing going for option 2 is that it makes sure the users know what's going on.

Currently, as you pointed out as well, users have no idea that ts-jest uses babel when allowSyntheticDefaultImports is set to true. It's also not very intuitive. If I came across something like this in another library, I'd be at my wits end should something related go wrong and I have to debug it!

It is a breaking change but the fix required at the users' end would be straightforward - set transformUsingBabel to true in ts-jest config. This would also give us an opportunity to introduce the ts-jest config within the jest config which can be used by #217 later.

[GeeWee]

I think the reason nobody knows what's going on is because it's not documented. I feel like option #1 would be easily explained with something like:

'If allowSyntheticDefaultImports is set to true, jest-ts automatically uses babel to transpile es6 modules to commonJS modules. You can opt out of this behaviour by setting skipBabel to true'

[kulshekhar]

Sounds good

[kulshekhar]

@shlomiassaf I've sent a PR to your repo. Can you confirm that this fixes your issue?

[jupl]

I'm confused on the conclusion. My understanding is that right now skipBabel is only applicable to allowSyntheticDefaultImports. If that's the case is the plan to eventually make skipBabel be independent of allowSyntheticDefaultImports?

[kulshekhar]

@jupl babel is used internally by ts-jest only when allowSyntheticDefaultImports is set to true so skipBabel is really meaningless without it

[GeeWee]

Currently babel is only used when allowSyntheticDefaultImports is set to true, so it covers all cases. I imagine if we end up using babel for every transformation we will make them independent

On Wed, May 24, 2017, 20:43 jupl ***@***.***> wrote: I'm confused on the conclusion. My understanding is that right now skipBabel is only applicable to allowSyntheticDefaultImports. If that's the case is the plan to eventually make skipBabel be independent of allowSyntheticDefaultImports? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGFg4MgaQUT_E7v11y95og3xGJzpGtnjks5r9Ho1gaJpZM4NaeJh> .

-- mvh. Gustav Wengel