Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid transpile the TypeScript code twice. #410

Merged
merged 9 commits into from Jan 5, 2018

Conversation

lijunle
Copy link
Contributor

@lijunle lijunle commented Jan 4, 2018

@kulshekhar It works for me...

BTW, I am not sure why my pre-commit style check convert the space to 4. 馃

@kulshekhar
Copy link
Owner

kulshekhar commented Jan 4, 2018

@lijunle Thanks for the PR :)

I'll try to take a look at this after work today.

btw, could you also:

  • bump the version patch in package.json so that this can be published right away
  • update the AUTHORS file with your name/email in alphabetical order?

kulshekhar
kulshekhar previously approved these changes Jan 5, 2018
Copy link
Owner

@kulshekhar kulshekhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kulshekhar kulshekhar requested a review from GeeWee January 5, 2018 11:28
src/utils.ts Outdated
// See
// https://github.com/Microsoft/TypeScript/blob/a757e8428410c2196886776785c16f8f0c2a62d9/src/compiler/sys.ts#L203 :
// tslint:disable-next-line:max-line-length
// See https://github.com/Microsoft/TypeScript/blob/a757e8428410c2196886776785c16f8f0c2a62d9/src/compiler/sys.ts#L203 :
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, this change can be reverted.

src/utils.ts Outdated
const start = src.length > 12 ? src.substr(1, 10) : '';

const sourceMapHook = `require('ts-jest').install()`;
const sourceMapHook = `require('ts-jest').install(${JSON.stringify(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code style is strange.

export function injectSourcemapHook(
filePath: string,
typeScriptCode: string,
src: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typeScriptCode and src are related but different. I do need to understand what/why we need babel after TypeScript (later).

@lijunle
Copy link
Contributor Author

lijunle commented Jan 5, 2018

@kulshekhar @GeeWee I suppose the code is ready to merge. Howover, I could like to discuss why anybody need babel be after TypeScript transform. The typeScriptCode and src paramters for method injectSourcemapHook looks strange.

@kulshekhar
Copy link
Owner

@lijunle

like to discuss why anybody need babel be after TypeScript transform

The main reason for using babel after TypeScript is to support hoisting. Without hoisting support, users would be forced to place mock statements above imports

With the other changes in this PR, there's really no reason to leave this in even behind a flag
@kulshekhar kulshekhar merged commit 9eef99a into kulshekhar:master Jan 5, 2018
@kulshekhar
Copy link
Owner

@lijunle the PR has been merged and the latest version published on npm. Thanks again 馃槂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants