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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 10 commits into from

Conversation

zackschuster
Copy link
Collaborator

fixes #322

@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
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. 😑

eleith
eleith requested changes Sep 26, 2022
View reviewed changes
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!

zackschuster
zackschuster commented Oct 11, 2022
View reviewed changes
smtp/connection.ts
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.

288ec6b#diff-e24a4bfef4475b3b5d01828e660b3042f4fa02502c09864cd104c4418b235e4f

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 •
edited

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
Reviewers

@eleith eleith eleith approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

27 errors when building with @tsconfig/node18-strictest
4 participants
@zackschuster @PolRF @s0urfruit @eleith