Skip to content

Avoid transpile the TypeScript code twice.#410

Merged
kulshekhar merged 9 commits intokulshekhar:masterfrom
lijunle:ts-once
Jan 5, 2018
Merged

Avoid transpile the TypeScript code twice.#410
kulshekhar merged 9 commits intokulshekhar:masterfrom
lijunle:ts-once

Conversation

@lijunle
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread 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
Copy Markdown
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.

Comment thread 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
Copy Markdown
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.

Comment thread src/utils.ts
export function injectSourcemapHook(
filePath: string,
typeScriptCode: string,
src: string,
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants