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

Rely on jest sourcemaps #340

Closed
GeeWee opened this issue Oct 5, 2017 · 15 comments · Fixed by #554
Closed

Rely on jest sourcemaps #340

GeeWee opened this issue Oct 5, 2017 · 15 comments · Fixed by #554

Comments

@GeeWee
Copy link
Collaborator

GeeWee commented Oct 5, 2017

Jest is about to merge sourcemap support : jestjs/jest#3458

I think if possible we should rely on what they're doing there instead of replicating it ourselves.

It's not possible to act on this issue right now, but I'm just writing it now while I remember.
I'll try to update it when jest merges the sourcemap support in.

@kulshekhar
Copy link
Owner

Oh definitely! I have my eye on that as well

@SimenB
Copy link
Contributor

SimenB commented Jan 16, 2018

There are a couple of open PRs/issues for sourcemap in the jest repo (e.g. jestjs/jest#5257 and jestjs/jest#5177). Would love your input on those if applicable, and help testing that it works.

@kulshekhar
Copy link
Owner

@SimenB I'll try to find some time after work today to take a look.

@SimenB
Copy link
Contributor

SimenB commented Feb 11, 2018

We're about to remove the mapCoverage flag. I've tested a couple of repos using ts-jest, and coverage still looks correct. But I'd like to test some more if you have any larger projects using ts-jest?

After that you should be able to use Jest's own support for sourcemaps instead of rolling your own. At least that's the theory!

EDIT: Sourcemap support is still out in jest now, so you should be able to rely on Jest installing the source-map support into the environment anyways. The new changes just allows you to return a sooucemap without inlining it in the sourcecode, and drop the mapCoverage option.

@kulshekhar
Copy link
Owner

@SimenB I use ts-jest only for nodejs server side code (unfortunately not open source) and don't have access to a large-ish open source project that could be used here

I use angular and its default test setup for the frontend in my personal projects and Jest + Flow at work for the react project

That said, I think there have been instances here where folks have linked to relatively larger projects when asked for minimal reproducible repos. Let to try to find some. I'll list them here if I can find any that might help.

@kulshekhar
Copy link
Owner

@SimenB Does this look helpful - https://github.com/EYHN/react-typescript-kits?

@SimenB
Copy link
Contributor

SimenB commented Feb 11, 2018

Thanks! That went better than expected 🙂 It did not use mapCoverage, so it was off anyways. But adding mapCoverage showed the correct things, except for files which are untested.
This is jest 22.2.1 and mapCoverage in an untested file:

image

Vs running with jestjs/jest#5177:
image

So not only does it make coverage work out of the box - it works better than before. This is awesome.

(coverage in files actually under test works the same with mapCoverage and the new code)

@kulshekhar
Copy link
Owner

kulshekhar commented Feb 11, 2018

This is fantastic!

I'll take a closer look at this next weekend. I think its time to see how this can reduce lines of code in ts-jest 😃

It was discussed a couple of times and didn't go anywhere but I have a feeling the time might be right for jest to subsume ts-jest.

@SimenB
Copy link
Contributor

SimenB commented Feb 20, 2018

@kulshekhar jest 22.4.0 has been released, so mapCoverage now prints a deprecation warning.

@kulshekhar
Copy link
Owner

I've set aside time to bring ts-jest up to speed with this

@kulshekhar kulshekhar mentioned this issue Feb 25, 2018
@GeeWee
Copy link
Collaborator Author

GeeWee commented Mar 4, 2018

Judging from your PR - what remains to be done is:

  • check if we can stop generating inline sourcemaps. This will affect the compilation in the preprocessor (tsc) and in the postprocessor (babel)
  • check whether the sourcemap config for sourcemap-support and jest can be combined (currently the sourcemap is being passed to jest and also to sourcemap-support)

Is that still accurate @kulshekhar ?

@kulshekhar
Copy link
Owner

@GeeWee yes

@chrisfarms
Copy link

chrisfarms commented Mar 18, 2018

Think this is related: I'm currently having to hack the ts-jest module to disable the injection of the sourceMapSupport.install() hook to get correct correct line numbers in stack traces (ts-jest 22.4.1 + jest 22.4.2 + typescript 2.7.2, target:es2017, node 8.9.4)

.... I'm guessing that something else in the tree (jest/jasmine?) is already performing the source-map-support install hook now which doesn't play nice when it's called twice.

ts-jest seems to work fine and as expected without the injected code, so maybe it's just not needed anymore? either way it would be nice to be able to disable the sourceMapSupport injection via the global options.

@kulshekhar
Copy link
Owner

@chrisfarms I've added this flag (undocumented in the readme) in a new release. Can you see if that works for you? The release notes have some documentation: https://github.com/kulshekhar/ts-jest/releases/tag/v22.4.2

@chrisfarms
Copy link

@kulshekhar ... yep spotted the release good work thanks ... I have nice correct line numbers again when disableSourceMapSupport: true is set 🎉

GeeWee added a commit that referenced this issue 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.
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 a pull request may close this issue.

4 participants