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

build: fix type errors with newer versions of typescript #323

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

zackschuster
Copy link
Collaborator

fixes #322

@zackschuster zackschuster requested a review from eleith September 21, 2022 18:56
@zackschuster
Copy link
Collaborator Author

node@12 is failing consistently to timeout for macOS. i'd really like to be done with this business. i have an issue open with evidential findings but it has no traction.

@zackschuster zackschuster force-pushed the fix-post-tsc4.3-type-errors branch 2 times, most recently from c173f4b to 4298496 Compare September 22, 2022 01:58
@zackschuster zackschuster force-pushed the fix-post-tsc4.3-type-errors branch from 4298496 to dabdbbd Compare September 22, 2022 02:26
@zackschuster
Copy link
Collaborator Author

i have essentially given up trying to make this better and just configured ava to tolerate the slowness. tests are passing now. whoo. 😑

Copy link
Owner

@eleith eleith left a comment

Choose a reason for hiding this comment

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

left a few minor comments. overall looks good. i'm interested in hearing your responses to some of my questions.

smtp/address.ts Show resolved Hide resolved
smtp/address.ts Outdated Show resolved Hide resolved
smtp/connection.ts Show resolved Hide resolved
smtp/mime.ts Outdated Show resolved Hide resolved
@PolRF
Copy link

PolRF commented Oct 5, 2022

Please merge the PR!

Comment on lines -557 to +558
return (this.features ?? {})[opt.toLowerCase()] === undefined;
return this.features != null && this.features[opt.toLowerCase()] != null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eleith this behavior has been in place since before i even showed up (see 50b9ec1) but it just seems wrong

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

it took a bit of digging because this file has been renamed more than once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the only change i did to that method in that commit was to add nullish coalescing. i'm referring specifically to the comparison to undefined.

Copy link
Owner

Choose a reason for hiding this comment

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

agreed. it's wrong. i also can't find any use of this public method anywhere.

@s0urfruit
Copy link

heya... so we gonna get this merged?

@zackschuster
Copy link
Collaborator Author

zackschuster commented Oct 18, 2022

i need to update the changelog prior to merge, as it is badly out-of-date. we don't have any automation for this task so it takes a bit to manually gather & format the data and i haven't had the space.

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.

27 errors when building with @tsconfig/node18-strictest
4 participants