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

fix node 18 breakage #176

Merged
merged 4 commits into from
Apr 28, 2022
Merged

fix node 18 breakage #176

merged 4 commits into from
Apr 28, 2022

Conversation

zackschuster
Copy link
Contributor

setting the closed property is a fatal operation under node 18. removing the offending lines seems to make no difference for the tests. i've added a github actions workflow for running the tests under various nodes & fixed a test that was failing under windows.

@zackschuster zackschuster changed the title fix node 18 breakages fix node 18 breakage Apr 21, 2022
@sibelius
Copy link

why is it a fatal operation?

@zackschuster
Copy link
Contributor Author

i believe it's due to nodejs/node#40696 adding them to the official API

@niklaswall
Copy link

According to the documentation the read-only property called "closed" was added to the Writable class in v18.0.0 of nodejs. And it also looks like the property "_closed" is used internally in the SMTPConnection class. So this patch looks ok.

@rafipiccolo
Copy link

Thanks for the patch. May I ask, when will the new version be published ?

@andris9
Copy link
Member

andris9 commented Apr 28, 2022

The test suite coverage is very low, so removing .closed property does not break the tests. This does not mean you can just remove it. At the least, you should try to find where this property is actually used and make sure that the existing functionality is not broken with the PR.

@zackschuster
Copy link
Contributor Author

zackschuster commented Apr 28, 2022

@andris9 it is now a conditional set, achieved by consulting a new getter-only prop on SMTPStream that determines if the closed property is itself a getter-only prop. it does this by using the time-honored technique of returning false if prop assignment throws.

...of course, since it's a (likely tiny) perf hit, i'll stew on it for years until v16 goes EOL & i can re-submit the original changeset 😆

@andris9 andris9 merged commit a2e765f into nodemailer:master Apr 28, 2022
@zackschuster
Copy link
Contributor Author

thanks!

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.

5 participants