-
Notifications
You must be signed in to change notification settings - Fork 917
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
[BUG] Typescript compiler output disregarded as part of normal path build commands #1660
Comments
I wonder if we can add this to our coverage reporter and track it there? hat way we can make sure that our errors is steadily reducing instead of looking for specific error codes |
If you flatten this into a numeric metric you can't distinguish new vs old errors. An advantage of hitting tare on the scale and saying "today we have '0' errors" is we get to act like typescript is a crisp boolean thing going forward. We could do both and try to push the accepted errors to 0 over time... |
Could we use betterer to help us both prevent regressions and make incremental progress? https://dev.to/phenomnominal/stricter-typescript-compilation-with-betterer-dp7 I know it can help with migrating to stricter typescript configurations, but I think it could also work for this use case. |
[Triage]: add this in the mean time: #1745 |
I recently dove into this and noticed that a lot of type errors were introduced in new features such as #2334. This needs to be prioritized. |
Current state:
|
I'm going to attempt to fix as many of the current errors that I can and then create follow-up issues to address the remaining ones. I think that the best long-term approach here is to just fix the issues now and enforce type checks in the build moving forward. It seems like some of the recent new features and changes are introducing many errors. |
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
After #3022:
TelemetryAre we going to remove this plugin?
Errors not related to sub-issues
|
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
* Reduces number of errors from 517 across 260 files to 216 across 115 files (~58% reduction). * The remaining type errors are documented in opensearch-project#1660. Signed-off-by: Tommy Markley <5437176+tmarkley@users.noreply.github.com>
We have since added more type errors to our code. Here is an updated list:
|
You can run |
Standard build scripts run tsc but do not fail on or show typescript errors. This has led to a confusing relationship with typescript and allowed 700+ typescript compilation errors to creep into the project.
Expected:
You can demonstrate this by adding in the following erroneous code to a .ts file and trying any of the standard build commands (yarn start, yarn build, yarn osd bootstrap).
This should generate the error:
error TS2355: A function whose declared type is neither 'void' nor 'any' must return a value.
Actual:
No error is surfaced in the console, and the build commands proceed as if the code compiled correctly.
Research
It looks like we're effectively using typescript as a babel passthrough. There are a few scripts in src/dev/typescript that look for local tsconfig.json files and execute tsc inside those directories. I spent some time fiddling with these scripts yesterday and got to the point where I could run tsc against a new plugin with only 9 errors not originating in the new plugin, which feels much more tractable.
I did that by creating a local tsconfig.json inside the plugin and running:
It's not totally 💯 though since I did have to hard-code my baseUrl. More research needs to be done to see how we can run the typescript compiler targeted at plugins in an automated way.
Ideal State
I came into the project with the assumption that if I wrote incorrect typescript the project wouldn't build and the CI system would reject my changes. I think that's a very reasonable assumption, and we should try to meet it.
Suggested path forward
Since we have quite a few errors it's probably not tractable to fix them all to get back to a completely healthy state. Also, there may be other prohibitive factors that led previous maintainers to opt out of type checking.
I think we can quickly get to a place where we can opt plugins into type checking via config, then we can incrementally reintroduce type checking.
Another path would be to mark all errors today as known and prevent PRs from adding errors. This is probably cleaner in aggregate but I imagine it'd take more code to integrate into builds. It might be as easy as (pseudocode):
The text was updated successfully, but these errors were encountered: