-
Notifications
You must be signed in to change notification settings - Fork 453
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Jest sourcemaps and remove languageserver #554
Conversation
@@ -11,7 +11,7 @@ describe('use strict', () => { | |||
const stderr = result.stderr; | |||
|
|||
expect(result.status).toBe(1); | |||
expect(stderr).toContain('Strict.ts:4:4'); | |||
expect(stderr).toContain('Strict.ts:4:3'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look right. The error is on the 4th line and not the 3rd -
ts-jest/tests/use-strict/Strict.ts
Line 4 in 392551a
v = 33; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this - I'm still half asleep. Didn't realize it was the column number
import { transpileTypescript } from './transpiler'; | ||
|
||
export function process( | ||
src: string, | ||
filePath: Path, | ||
jestConfig: JestConfig, | ||
transformOptions: TransformOptions = { instrument: false }, | ||
): string { | ||
): CodeSourceMapPair | string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function no longer returns result as a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does if it's a file we're not processing, see the return src a little further down. It might have been possible just to return an empty sourcemap to avoid the union type, but I didn't want to risk it.
Can you confirm that source maps still work with this change? I couldn't find any test cases that verify the source maps are produced, but I might have missed them. I'm getting no source map text and no break points hit in IntelliJ with this change and TypeScript 2.9.2. I reverted back to ts-jest 22.4.2 and breakpoints worked again. I'm consistently getting an undefined Lines 19 to 22 in 227f307
Debugging into the TypeScript source code, a |
Are you building it from source? I was unaware this had even been released yet. it should emit a sourcemaptext: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#transpiling-a-single-file We have several tests that should break if sourcemaps aren't working. |
Thanks for the quick response. Yeah I'm building from sources, and pointing to the built sources using a file url in my project Results based on the checked out version of
So there appears to be something that breaks compatibility with IntelliJ/WebStorm between those two versions. I came across it as I was looking into adding support for custom transforms. I guess I'll use the working tag as a starting point. If I figure out a reproducible setup I'll create an issue, but I guess I'd have to wait for the next release. As for the undefined sourceMapText, that's identical between the two versions, so the issue lies somewhere else. Feel free to delete my comments here to cleanup this PR. Thanks, |
This PR uses the jest infrastructure for sourcemaps. It completely removes any dependance on source-map-support from our part.
Note that removing the languageServer is a breaking change - however it was never documented. A language-server branch has been created based off master.
This PR is based on #552 so merge that in first.
It closes #340. Surprisingly enough it also closes #240 - passing the sourcemaps explicitly to babel means that the line# are correct in the other end - you'll see that in one of the updated tests.
It fixes part of #529
Note that no line# has changed, but some column names have in the tests. I'm not sure the original column names were ever accurate.