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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Jest sourcemaps and remove languageserver #554

Merged
merged 7 commits into from
May 26, 2018
Merged

Conversation

GeeWee
Copy link
Collaborator

@GeeWee GeeWee commented May 26, 2018

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.

@GeeWee GeeWee changed the title WIP: Use Jest sourcemaps and remove languageserver Use Jest sourcemaps and remove languageserver May 26, 2018
@@ -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');
Copy link
Owner

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 -

Copy link
Owner

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

@GeeWee GeeWee merged commit 0d07a9d into master May 26, 2018
@GeeWee GeeWee deleted the jest-sourcemaps branch May 26, 2018 14:01
import { transpileTypescript } from './transpiler';

export function process(
src: string,
filePath: Path,
jestConfig: JestConfig,
transformOptions: TransformOptions = { instrument: false },
): string {
): CodeSourceMapPair | string {
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@jimkyndemeyer
Copy link

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 sourceMapText back from TypeScript at:

return {
code: transpileOutput.outputText,
map: transpileOutput.sourceMapText,
};

Debugging into the TypeScript source code, a '.map' file is not emitted, so the sourceMapText is never assigned in:

https://github.com/Microsoft/TypeScript/blob/a56b272d3891916a8debe36087fe7563bbade164/src/services/transpile.ts#L79-L83

@GeeWee
Copy link
Collaborator Author

GeeWee commented Jun 14, 2018

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.
I'd prefer not to keep the discussion here in the PR (because i'll forget it at some point and it'll be here forever) - would love it if you opened an issue

@jimkyndemeyer
Copy link

Thanks for the quick response.

Yeah I'm building from sources, and pointing to the built sources using a file url in my project package.json.

Results based on the checked out version of ts-jest:

  • master: breakpoints never hit in IntelliJ
  • tag 22.4.4: breakpoints working again in IntelliJ

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,
Jim.

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.

Rely on jest sourcemaps jest.mock() calls underneath the code mangle the stacktraces
4 participants