search

nodemailer / smtp-server

Create custom SMTP servers on the fly
Other
862 stars 146 forks source link

fix node 18 breakage #176

Closed zackschuster closed 2 years ago

zackschuster commented 2 years ago

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.

sibelius commented 2 years ago

why is it a fatal operation?

zackschuster commented 2 years ago

i believe it's due to https://github.com/nodejs/node/pull/40696 adding them to the official API

niklaswall commented 2 years ago

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 commented 2 years ago

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

andris9 commented 2 years ago

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 commented 2 years ago

@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 😆

zackschuster commented 2 years ago

thanks!