New pipeline #236
New pipeline #236
Conversation
| } | ||
|
|
||
| export interface PostProcessHook { | ||
| (src: string, filename: string, config: JestConfig, transformOptions: TransformOptions) : string; |
There was a problem hiding this comment.
Would a new type to pass into this function be better than multiple params? It'll give a bit of flexibility if we want to change something. For instance, we might want to pass in the original source, the transpiled output and a sourcemap.
There was a problem hiding this comment.
I'm not opposed to the idea - this is just a rough draft. Does it make more sense to change the type now, or change it if/when we know what users want? I'm a little vary of overengineering.
There was a problem hiding this comment.
fair enough. We'll leave this as is for now
src/postprocess.ts
Outdated
| @@ -0,0 +1,56 @@ | |||
| /** | |||
| * Postprocess step. Stolen from babel-jest: https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js | |||
There was a problem hiding this comment.
'Based on' sounds better than 'Stolen' :)
Also, it might be better to use a link of a particular commit. So even if the file changes in the future, we'll have a reference to what this code was based on.
There was a problem hiding this comment.
Haha true. Yes that's a good idea.
src/preprocessor.ts
Outdated
| const outputText = | ||
| compilerOptions.allowSyntheticDefaultImports && !tsJestConfig.skipBabel | ||
| ? babelJest.process( | ||
| const outputText = postHook( | ||
| tsTranspiled.outputText, | ||
| path + '.js', // babel-jest only likes .js files ¯\_(ツ)_/¯ |
There was a problem hiding this comment.
Should we take the opportunity to change the extension based on the source file? If it's .tsx, set it to .jsx otherwise set it to .js?
There was a problem hiding this comment.
Damnit, I realised this I had forgotten this just as I left.
Actually to fix #211 we should just not modify the path - we don't need to fix it to pass babel's typechecks anymore.
There was a problem hiding this comment.
Does that mean we don't need to add .js any more?
There was a problem hiding this comment.
Yep!
Edit: Or at least I'm 95% sure.
src/preprocessor.ts
Outdated
| @@ -30,6 +27,7 @@ export function process( | |||
| const isTsFile = path.endsWith('.ts') || path.endsWith('.tsx'); | |||
| const isJsFile = path.endsWith('.js') || path.endsWith('.jsx'); | |||
| const isHtmlFile = path.endsWith('.html'); | |||
| postHook = getPostProcessHook(compilerOptions, config, tsJestConfig); | |||
There was a problem hiding this comment.
minor nit - indentation.
should there be a if postHook !== undefined around this? If not, is there any reason to declare it outside this function?
There was a problem hiding this comment.
Good catch!
No, that's an oversight - also a good catch!
|
Will get around to fixes tomorrow I think |
|
@GeeWee If you can respond to my subsequent queries - I can push these changes in and you can review it tomorrow or when you have time |
|
Definitely - @kulshekhar, that'd be awesome. I think I got around to all of them - let me know if you need anything more |
|
Looks good to me atm. |
|
Well, the tests check it indirectly. For example, https://github.com/kulshekhar/ts-jest/blob/master/tests/__tests__/ts-coverage.spec.ts checks for the coverage which would only work if the |
| } | ||
|
|
||
| //If we're not skipping babel | ||
| if (tsCompilerOptions.allowSyntheticDefaultImports) { |
There was a problem hiding this comment.
This indicates that babel won't be used if allowSyntheticDefaultImports is not set to true?
There was a problem hiding this comment.
Correct, and I want it to, but this necessitates changing tests, as using babel changes the coverage numbers slightly (the statement count is off by one) and I'd like to merge this in before making new changes that'll also necessitate changing tests.
There was a problem hiding this comment.
So once tests are updated, babel will always be used unless skipBabel is set, right?
If that's the case, open an issue so that we don't forget that, increase the version of ts-jest and merge it in. I'll publish it right away after that
There was a problem hiding this comment.
I think #227 covers it?
There was a problem hiding this comment.
sounds good. Ping me when it's ready to publish
|
Yes I see, that makes sense. I'd like some direct tests at one point, but baby steps. I'm ready to merge this in if you are. |
To resolve #235
Added 'json' moduleFileExtension, otherwise some tests fail.
I ported the babel-jest to our pipeline and started on something that might become a hook architecture at some point.
Would love to hear your thoughts.