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

WIP [poc] get rid of ts-node #2563

Closed
wants to merge 3 commits into from
Closed

Conversation

akosyakov
Copy link
Member

It is not to merge but compare results.

I've noticed that our tests are running slow during the build, because we recompile them with ts-node, even for packages which don’t have any. More dependencies package has more time it takes, to compare for @theia/core:

  • Done in 36.86s. - with ts-node
  • Done in 12.20s. - without ts-node

Tests execution itself takes 8-9s. All other packages take even longer.

Also, latest ts-node does not really work with our setup. They don’t look at files attribute in tsconfig anymore to speed up tests, but it breaks us since we specify there how d.ts files should be resolved.

I wonder whether we can get rid of it. Does anybody object?

Test execution form VS Code and from cli would be faster as well, plus no need to struggle with ts-node during switching to newer ts version. The only drawback that we should watch the package under testing, but most of the time it is the case.

@akosyakov
Copy link
Member Author

It is 20min 49sec on master vs 14min 21sec with this PR.

@svenefftinge
Copy link
Contributor

For me personally, it is fine (and I even prefer) to have a tsc watching instead of relying on ts-node.
I somehow don't fully trust that it does exactly the same thing :)

@paul-marechal
Copy link
Member

paul-marechal commented Aug 13, 2018

What do you mean by "we should be watching the tests when testing"?

After reading a bit, my understanding is that we currently didn't build the tests, but rather interpreted the Typescript files (.spec.ts) using ts-node, which evaluates typescript on the fly. Couldn't we build everything in a first pass (sources + tests) and then directly use the result?

edit: I would definitely like this optimisation, as your benchmarks are very interesting!

@svenefftinge
Copy link
Contributor

Couldn't we build everything in a first pass (sources + tests) and then directly use the result?

Yes, that is what you do without ts-node. But since it adds a step to your turnarounds some developers prefer ts-node. But I think tsc watch is just as good but closer to what the build does.

@akosyakov
Copy link
Member Author

akosyakov commented Aug 13, 2018

What do you mean by "we should be watching the tests when testing"?

when you work on a package, then you should recompile it before running tests

After reading a bit, my understanding is that we currently didn't build the tests, but rather interpreted the Typescript files (.spec.ts) using ts-node, which evaluates typescript on the fly. Couldn't we build everything in a first pass (sources + tests) and then directly use the result?

It is what happens with this PR during the build and why it is faster.

@paul-marechal
Copy link
Member

paul-marechal commented Aug 13, 2018

Ok I understand better now, I was worried about starting tsc watch on the travis build... While actually, only developers would watch the extension they work on, while travis is already configured to run spec.js files: it sounds perfect!

@paul-marechal
Copy link
Member

It is not to merge but compare results.

How about mentioning these results on the dev meeting? We can add it to the agenda to know if people would mind or not having to build their tests using tsc watch before hand. I feel like this could go in as-is, given everyone is ok with this PR.

@akosyakov
Copy link
Member Author

@marechal-p sounds good with the dev meeting, this PR will need some love to completely get rid of ts-node

@akosyakov
Copy link
Member Author

akosyakov commented Aug 13, 2018

Also, after we switch to project references with ts 3, watching of the whole repo should be super fast.

@kittaakos
Copy link
Contributor

step to your turnarounds some developers prefer ts-node

I personally prefer ts-node over watching the changes with tsc but since we can not run most of the mocha tests in watch mode due to inversify anyway, +1 for performance.

@kittaakos
Copy link
Contributor

@akosyakov, can I still test and debug one single module from VS Code after this PR gets merged to master?

@akosyakov
Copy link
Member Author

@kittaakos I will need to look how it can be done, but we for sure want to be able to debug the single module from VS Code.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov
Copy link
Member Author

akosyakov commented Jan 2, 2019

@kittaakos I've figured out how to debug Mocha tests without ts-node: https://github.com/akosyakov/ts-mocha-debugging

Also, after we switch to project references with ts 3, watching of the whole repo should be super fast.

Regarding this. Generally, it improves subsequent compilations, but watching in the build mode all packages is still a way slower than watching for an individual package without package references. I won't incorporate it into watch npm script because of it, only in the build script.

@akosyakov
Copy link
Member Author

akosyakov commented Jan 22, 2019

I will open separate PRs for getting rid of ts-node and enabling project references. Please don't touch the branch though i need it.

@akosyakov akosyakov closed this Jan 22, 2019
@akosyakov akosyakov deleted the ak/get_rid_of_ts_node branch February 28, 2019 15:47
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.

None yet

4 participants