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

Create Roadmap and Architecture plan for this plugin #270

Closed
piotr-oles opened this issue Apr 25, 2019 · 6 comments
Closed

Create Roadmap and Architecture plan for this plugin #270

piotr-oles opened this issue Apr 25, 2019 · 6 comments
Assignees

Comments

@piotr-oles
Copy link
Collaborator

As we discussed in the #257, we need a Roadmap to plan future work on this plugin. Here is the place to share our ideas and drafts :)

@phryneas
Copy link
Contributor

phryneas commented Apr 27, 2019

So... I'll try and gather some thought.

Step 1: Tests

Honestly, after #253 and #268, where I discovered that many tests were just testing nothing my first priority would be to get the test suite into a better state.
(Something along the lines of there were tests testing for 0 errors after a compile, but they initialized against a folder without sources in it, so they would always get 0 errors)

Things I am thinking about:

  • evaluate if we could benefit from a switch to jest
    I'm seeing two possible wins here: maybe a speed gain and the ability to write tests in TypeScript. The API between both is almost interchangeable, so there's no high cost in switching.
  • split up the test suite into more files
    the index.spec.js is getting really big...
  • refactor the helper functions
    there is a lot of duplicate code here, and very much of it has gone out of sync
  • comment all tests
    there are tests like the one in improve generation of diagnostics, fix linter tests #268 that seem to serve no purpose until you git blame and notice that is is a test for a regression. I would like to have every test annotated with the what it tests and especially the why
  • provide a counter-check for most tests
    to prevent the situation above where a test would always succeed because it already failed in setting up the test, all tests that should look for 0 errors should be accompanied by a counter-check test that provokes an error
  • execute less tests on every PR/commit and more tests on a cronjob basis
    since we now have the beta branch I guess we could dial back from executing the test suite 16 times for every PR to maybe 4 times or something. As a trade-of, we could run even more tests that we are currently running on a daily basis against the beta branch

Step 2: Move vue-specific logic out of IncrementalChecker and ApiIncrementalChecker

This is essentially #231

Step 3: evaluate if we still need IncrementalChecker and ApiIncrementalChecker

Currently, only IncrementalChecker can be executed in multiple processes at once and as long as that isn't going to change, I guess we've got to keep both? Also, what are other differences?

Step 4: make tslint and eslint pluggable

That way we could support both, without too much overhead.

What are your thoughts?

@johnnyreilly
Copy link
Member

Agree with all points 😀

Currently, only IncrementalChecker can be executed in multiple processes at once and as long as that isn't going to change, I guess we've got to keep both? Also, what are other differences?

I'd rather just have one checker. I'd rather that checker was ApiIncrementalChecker as it's faster. That would mean dropping support for < TS 2.9. I am fine with that. I don't think the multi process IncrementalChecker thing is advantageous when you compare to the speed of the ApiIncrementalChecker.

@phryneas
Copy link
Contributor

@johnnyreilly in #257 you said that the ApiIncrementalChecker does not support everything the IncrementalChecker does.
Besides vue support and multithreading - are there any other things that are still missing?

As for timing: I've got three rough weeks ahead of me. I'll try to sneak some work on the tests in, but bigger chunks of work will have to wait until after mid-may.

@johnnyreilly
Copy link
Member

johnnyreilly commented Apr 28, 2019

@johnnyreilly in #257 you said that the ApiIncrementalChecker does not support everything the IncrementalChecker does.
Besides vue support and multithreading - are there any other things that are still missing?

We need Vue support. I don't think we need multithreading. When @0xorial contributed the ApiIncrementalChecker #198 I believe there was some conversation around why it wasn't feasible / advantageous to have the multi-threading. Either way; we don't need it I think.

As for timing: I've got three rough weeks ahead of me. I'll try to sneak some work on the tests in, but bigger chunks of work will have to wait until after mid-may.

Dude - it's open source. It's done for love and laughs when we have time. We totally ❤️ and appreciate all contributions that people are able to make. We're really grateful! It's important that you know that there are no expectations on you to do certain things by a certain time. It's more like this: if you do stuff that is amazing! We'll be delighted. 😀 But don't feel bad if you don't get chance. We all have busy lives 🌻😎

Oh and on TSLint - given it is deprecated I'm cool with a future that doesn't necessarily support it in fork-ts-checker-webpack-plugin. But only when we've got support for eslint added. Some useful details here: #203

@piotr-oles
Copy link
Collaborator Author

@phryneas - good job! :) I agree with @johnnyreilly that multi-threading is a something we can give up. To make multi-threading type-checking reasonable, there would be needed changes in the TypeScript architecture. The required changes are:

  1. serialization friendly TypeScript AST (so called SourceFile)
  2. asynchronous API in CompilerHost (so we could share SourceFiles between threads using for example redis)
    These was changes that I came up 2 years ago - maybe they are not up to date :)

@piotr-oles
Copy link
Collaborator Author

I will close this issue to clean up the backlog :) Most of the points were implemented in the v5.0.0-alpha-1 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants