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

Turn on noImplicitAny and fix all errors #3845

Merged
merged 18 commits into from
Aug 13, 2023
Merged

Conversation

bmakuh
Copy link
Contributor

@bmakuh bmakuh commented Jul 28, 2023

As part of the conversion of the codebase to TypeScript, this PR turns on the TS noImplicitAny flag and fixes all the type errors surfaced as a result. A vast majority of them were fixed with type annotations, however there are quite a few areas where real code changes were made to fix things such as nullability problems or parameter type mismatches. Perhaps the most serious of which is inside the table embed, where the existing types assumed rows and cells were op arrays but the code was passing around full deltas.

Feedback welcome!

Copy link
Member

@luin luin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

blots/scroll.ts Outdated Show resolved Hide resolved
blots/block.ts Outdated Show resolved Hide resolved
blots/block.ts Outdated Show resolved Hide resolved
blots/scroll.ts Outdated Show resolved Hide resolved
blots/scroll.ts Outdated Show resolved Hide resolved
test/unit/__helpers__/expect.ts Outdated Show resolved Hide resolved
modules/toolbar.ts Outdated Show resolved Hide resolved
formats/indent.ts Outdated Show resolved Hide resolved
core/theme.ts Outdated Show resolved Hide resolved
modules/tableEmbed.ts Outdated Show resolved Hide resolved
@bmakuh
Copy link
Contributor Author

bmakuh commented Jul 30, 2023

Thanks for the feedback! I still have some failing tests that I plan to address just FYI

core/quill.ts Outdated Show resolved Hide resolved
@bmakuh
Copy link
Contributor Author

bmakuh commented Aug 3, 2023

@luin tests and lint should all be passing if you want to run the workflow again

Copy link
Member

@luin luin left a comment

Choose a reason for hiding this comment

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

Thanks for the update. I think we should focus on changes to types in this PR and try to avoid changes to actual code when.

For changes to types, we can focus on function signatures and use // @ts-expect-error TODO fix this later inside function unless it's a easy fix.

test/unit/__helpers__/expect.ts Outdated Show resolved Hide resolved
@@ -12,11 +12,17 @@ export type TableRowColumnOp = Omit<Op, 'insert'> & {
};

export interface TableData {
rows?: Delta['ops'];
columns?: Delta['ops'];
rows?: Delta;
Copy link
Member

Choose a reason for hiding this comment

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

We should not change the declaration of a public module. Here Delta should be a Delta['ops'].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that tableEmbed.ts is internally inconsistent. Functions like compactTableData() take in a TableData type but the code expects rows and columns here to be Deltas (it calls rows.length() instead of checking rows.length, as we would expect if the type really was an array of ops). So we have two options here:

  1. Change the declaration of the public module to match what the code actually does
  2. Change the code to match the public declaration

I originally went with option (2), but you asked me to change it to pass in Deltas. So we need to decide which of those two options we want to go with and I'm happy to make the changes, but the point is that something will need to change here as the type annotations are revealing.

Copy link
Member

Choose a reason for hiding this comment

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

I added the missing types without changing the actual code.

core/quill.ts Outdated Show resolved Hide resolved
core/selection.ts Outdated Show resolved Hide resolved
core/quill.ts Outdated
if (range == null) return null;
let start;
let end;
if (index && typeof index.transformPosition === 'function') {
if (index && index instanceof Delta) {
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid changing actual code. This brings a breaking change when users provide another Delta library or instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change it back to check for the typeof the transformPosition function, but we will at least need to add a check here that the typeof index !== 'number' (because index is either a number or a Delta at this point and we don't know which). Without a better type guard there, we get this error:

Property 'transformPosition' does not exist on type 'number | Delta'.
  Property 'transformPosition' does not exist on type 'number'. ts(2339)

I get the desire to avoid code changes and I did in fact try to avoid them wherever possible, but I erred on the side of adding better guards instead of just suppressing type errors. Our bug tracker at work reports tens of thousands of exceptions from Quill every day and part of my motivation with this PR was to find and fix the source of those 😄

Copy link
Member

Choose a reason for hiding this comment

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

Yeah feel free to create issues so I can take a look at the errors. We could address them in separate PRs.

core/selection.ts Outdated Show resolved Hide resolved
@@ -52,36 +58,39 @@ export const composePosition = (delta: Delta, index: number) => {
return newIndex;
};

const compactCellData = ({ content, attributes }) => {
const compactCellData = ({ content, attributes }: CellData) => {
Copy link
Member

Choose a reason for hiding this comment

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

We expect content to be a Delta.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment here: #3845 (comment)

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -47,15 +47,15 @@ describe('tableHandler', () => {
{
insert: {
'table-embed': {
rows: [
rows: new Delta([
Copy link
Member

Choose a reason for hiding this comment

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

Tests should not be changed unless it's related to types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment here: #3845 (comment)

@luin luin force-pushed the no-implicit-any branch 2 times, most recently from 27839b8 to 5ad404d Compare August 13, 2023 03:56
@luin luin force-pushed the no-implicit-any branch 5 times, most recently from 8adfcf7 to a7497e7 Compare August 13, 2023 04:31
@luin luin merged commit c5fa826 into slab:develop Aug 13, 2023
5 checks passed
@luin
Copy link
Member

luin commented Aug 13, 2023

Thanks for the contribution!

@bmakuh bmakuh deleted the no-implicit-any branch August 14, 2023 15:03
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.

2 participants