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

Move to language service compiler api #362

Closed
wants to merge 4 commits into from

Conversation

goloveychuk
Copy link

@goloveychuk goloveychuk commented Nov 5, 2017

#281
also makes easy to fix #303

Notes:

  • 3 tests are failing (1 one them was failing in master (about button))
  • I guess when jest works on concurrency mode it creates preprocessor.js context for every process (or even reimport it on overy transpile). If yes, this means that we have multiple compiler instances, so it will recompile same files for every jest worker. It could be optimized by using shared DocumentRegistry (https://github.com/Microsoft/TypeScript/blob/master/lib/typescript.d.ts#L4465)
  • I'm not sure why source maps implemented this way, but it worth to mention, that with sourceMap=true in CompilerOptions, on calling service.getEmitOutput we can get sourcemaps. We can push it to some cache and on install() use
  options.retrieveSourceMap = function(path) { return sourceMapCache.get(path) };

I guess this way we can avoid second run of transpileModule.

@goloveychuk
Copy link
Author

goloveychuk commented Nov 5, 2017

other note:
I'm not sure why, but with allowJs=true it starts to write

    Errors while compile ts:
      Error: Cannot write file '/<somepath>/ts-jest/dist/utils.js' because it would overwrite input file.
      Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.

It looks like ts compiler trying to write js (and not ts, what is interesting) files to fs.
If I'll provide "outDir" and remove line
delete config.outDir; in utils.ts everything is ok.
But this is just workaround, should be some option to avoid writing js files

UPD:
https://github.com/Microsoft/TypeScript/blob/8d5b0529b2bb7650d471b4d64fd88061cde4707d/src/compiler/program.ts#L2203
So basically js files are not emitting to fs. verifyCompilerOptions() called on program creation, not emitting. This is actually a ts bug, because filenames check is useless in case of emitting to string. And, as I understood, it doesn't warn for ts files since file extention is changed .ts -> .js
Should ping ts team about this.

@kulshekhar
Copy link
Owner

I'm not sure why source maps implemented this way, but it worth to mention, that with sourceMap=true in CompilerOptions, on calling service.getEmitOutput we can get sourcemaps. We can push it to some cache and on install() use

I think that the cache (which would be created at compile time) will not be available when install is called (at run time, likely in another process).

I'm not sure why, but with allowJs=true it starts to write

The compiler transpiles the ts-jest files to js and puts them in the dist directory. This should only happen when installing ts-jest - not when running tests.

If I'll provide "outDir" and remove line

I think there's an issue which required outDir to be removed.

@goloveychuk
Copy link
Author

The compiler transpiles the ts-jest files to js and puts them in the dist directory

looking at typescript source code, it's not emitting js code to fs. It just makes checks for filenames on program creation. Which is not valid for custom writer, which in our case is pushing to array (https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/compiler/builder.ts#L19-L26)

So in this case we can just override suppressOutputPathCheck=true in options.
Also created issue microsoft/TypeScript#19755

@kulshekhar
Copy link
Owner

kulshekhar commented Nov 6, 2017

I think you might be conflating two issues here. The javascript files in the dist directory (within the ts-jest package) are created when ts-jest is installed. These files should not affect the actual tests (which the project using ts-jest would be running).

Either this, or I'm likely missing something.

@goloveychuk
Copy link
Author

goloveychuk commented Nov 6, 2017

Look like some missunderstanding here. So in this pr

  1. ts-jest build phase is ok, js files are on dist. Any error. Everything right.
  2. when tests are transpilling:
    let's say we have some .ts module, which imports .js module. And we have allowJs in compiler options.
    When I'm using running
    https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/services/services.ts#L1064
    it runs https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/services/services.ts#L1187
    which runs
    https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/compiler/program.ts#L669
    which runs
    https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/compiler/program.ts#L2207
    which runs
    https://github.com/Microsoft/TypeScript/blob/8fc651870e44efd12598a42ae84fb0e19067fcfd/src/compiler/program.ts#L2227

So when I'm creating languageService for future emitting - it checks if it can write js files to fs.
But this is not relevant, since in future when I'm running getEmitOutput, result is going to string, not fs. So this is like ts bug.
And best option, for now, to workaround it - override suppressOutputPathCheck=true.

This all is relevant for tests transpiling step, not building ts-jest itself.

@kulshekhar
Copy link
Owner

This all is relevant for tests transpiling step, not building ts-jest itself.

that clears things a bit. Thanks.

So when I'm create languageService for future emitting - it checks if it can write js files to fs.
But this is not relevant, since in future when I'm running getEmitOutput, result is going to string, not fs. So this is like ts bug.

So if I understand this right, the issue is that the new API that this PR uses to transpile is trying to generate js files.

So this is like ts bug.

Doesn't setting noEmit to true address this?

And best options, for now, to workaround it - override suppressOutputPathCheck=true

If we're going to use a new API, we should avoid using workarounds - at least for the core parts.

@goloveychuk
Copy link
Author

So if I understand this right, the issue is that the new API that this PR uses to transpile is trying to generate js files.

it doesn't trying. It emitting file to string. Typescript thinks that it will emit js files to fs, so it runs some general checks, one of them is irrelevant in out case.
And it runs this check before emitting files, but on program creation.

Doesn't setting noEmit to true address this?

with noEmit with running getEmitOutput we will have empty data.

@kulshekhar
Copy link
Owner

it doesn't trying. It emitting file to string. Typescript thinks that it will emit js files to fs, so it runs some general checks, one of them is irrelevant in out case.
And it runs this check before emitting files, but on program creation.

I see. I now understand the issue. This does sound like a bug in ts.

with noEmit with running getEmitOutput we will have empty data.

Right, of course.

@goloveychuk
Copy link
Author

goloveychuk commented Nov 6, 2017

so still problems are

  1. smth wrong with sourcemaps, let's discover it later
  2. performance issues. With -w 8 --no-cache my tests are running 180s, with -w 1 --no-cache - 80s. Looks like for every worker ts compiler recompile same files. So options are:
  • make shared DocumentRegistry.
  • if it wouln't help - move compiler to child process and communicate inproc. Very unfortunate options, since it will make things difficult

@kulshekhar
Copy link
Owner

This seems like it would entail some serious restructuring.

While any improvement would be welcome, you might want to consider if it's worth your time. It seems that Babel 7 will have typescript support and depending on how that is done, ts-jest itself might not be required in future (See #358)

That said, any improvement is welcome here!

@goloveychuk
Copy link
Author

In my case babel-typescript-transformer doesn't work, since I'm using my own written customTransformer, which is injected in build process, traverse parse tree and make some modifications. This is not possible with babel. So I'll try to optimize this pr.

@kulshekhar
Copy link
Owner

@goloveychuk is this still an active issue?

@incleaf
Copy link

incleaf commented Jan 23, 2018

#281 is still an active Issue which is a valid use case. I think we should fix this problem in some way like this PR.

@kulshekhar
Copy link
Owner

@incleaf I asked because this PR has been inactive for 2+ months.

@goloveychuk
Copy link
Author

so I'm using it in my project - works ok, but have a big problem.

With using transform function (you're using currently) which get's only one file's source code, and it's easy to paralellize transforming.

language service get's all project context and checks everything, and from what I know and tried - it can't be parallelized.

@kulshekhar
Copy link
Owner

@goloveychuk yeah, it's a tricky issue.

Anyway, I was just checking. I don't mind leaving this PR open if it is still of interest to you :)

@goloveychuk
Copy link
Author

So maybe leave it open, mb someone find it useful and help with concurrency problem. Or maybe I'll fix it when have free time. Or close it, and reopen when needed. Your choice)

@kulshekhar
Copy link
Owner

@goloveychuk As long as you're interested in this, it can be left open.

I don't mean it has to be active with updates. Just that you are the owner of this PR and its status is completely up to you 😄

if (file !== undefined) {
return file.snapshot;
}
if (!fs.existsSync(fileName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how bad this would be for large apps with loads of files.
But maybe in order to avoid an extra IO operation you can do

try {
  return ts.ScriptSnapshot.fromString(fs.readFileSync(fileName).toString()); 
} catch {
  return undefined;
}

}

private formatErrors(fileName: string) {
let allDiagnostics = this.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Though maybe you can test it on a system with loads of files? I was trying to do this as well over the past 2 days, and so far tests where like 10x slower when you have a lot of files and resolutions let's say over 5K files.

I can through download your branch and test this out, if you'd like. Probably tomorrow.

Copy link
Author

Choose a reason for hiding this comment

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

I think this is not main problem.
In ts-jest master currently you have smth like
fileSources.map(s => transpileModiule(s)
And transpile module don't compile any imported files. This means that it could be easilly paralellized.
With program api it looks like we can't paralelize build. You could try to run tests with this branch - it works faster in single thread mode, than with multithreading. It recompiling same files in different processes. I don't know how to fix this. Maybe it could be common shared storage of snapshots.

@GeeWee
Copy link
Collaborator

GeeWee commented May 27, 2018

Closing due to inactivity. Feel free to reopen

@GeeWee GeeWee closed this May 27, 2018
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.

Using ts-jest together with ts-transformer-keys
5 participants