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

minification is messing up node-fetch's detection of abort signals #153

Closed
kentcdodds opened this issue May 17, 2021 · 12 comments · Fixed by #181
Closed

minification is messing up node-fetch's detection of abort signals #153

kentcdodds opened this issue May 17, 2021 · 12 comments · Fixed by #181

Comments

@kentcdodds
Copy link

I'm using nodemailer-mailgun-transport. When attempting to call transporter.sendMail, I'm getting:

TypeError: Expected signal to be an instanceof AbortSignal

The stack trace is kinda useless because it's minified. Also, because mailgun-js bundles all its deps with webpack, it's pretty difficult to find, but I'm pretty sure here's where the mailgun-js code gets involved: https://github.com/mailgun/mailgun-js/blob/28c71ded8b15dfa84d5933c9d458b834aa952d7a/lib/request.ts#L76

After investigating, I realized that node-fetch attempts to validate whether signal is an AbortSignal based on the name of the constructor.

In the latest stable version of node-fetch:

https://github.com/node-fetch/node-fetch/blob/v2.6.1/src/request.js#L42

And in the v3 beta it's still based on the name, but is implemented slightly differently:

https://github.com/node-fetch/node-fetch/blob/v3.0.0-beta.9/src/utils/is.js

In any case, the minification of mailgun-js is causing this issue.

Suggested solution

At the very least, don't minify. This would solve the problem. Also, you may have a reason to bundle, but that's pretty unusual so I'm curious why you're doing it. Can you avoid it? In my experience people handle bundling themselves for stuff like this.

@kentcdodds kentcdodds changed the title minification is messing up node-fetch's detection of abort controller minification is messing up node-fetch's detection of abort signals May 17, 2021
@kentcdodds
Copy link
Author

Hold it. I just realized I have the wrong package... nodemailer-mailgun-transport uses https://npm.im/mailgun-js and this is https://npm.im/mailgun.js 🤦

Still, the minification and bundling this project is probably not great 😅

@kentcdodds
Copy link
Author

Scratch that last comment... nodemailer-mailgun-transport is using this package (https://npm.im/mailgun.js): https://github.com/orliesaurus/nodemailer-mailgun-transport/blob/9fb9efd35a6113aa9f2ab7d9225091987a09d9a1/package.json#L39

Sorry, the naming of this stuff is pretty confusing. Perhaps consider updating the repo name to mailgun.js to reduce confusion? 😅

@kentcdodds
Copy link
Author

Just realized why this was working a few weeks ago and suddenly stopped working. 8 days ago, nodemailer-mailgun-transport switched from the old mailgun-js to this mailgun.js: orliesaurus/nodemailer-mailgun-transport@9c6596a

So yeah, anyway, I'm not sure why nobody else has reported this issue because from what I can tell, this shouldn't work for anyone using mailgun.js on the server.

@jjranalli
Copy link

Just stumbled upon this same issue and I can confirm mailgun.js appears to be currently unusable with nodemailer-mailgun-transport. Can also confirm the naming of this package is very confusing.

@weyert
Copy link

weyert commented May 19, 2021

I am having the same issue

@maxfi
Copy link

maxfi commented May 23, 2021

Related #101. The workarounds don't seem to be ideal though...

@tobimori
Copy link

tobimori commented May 23, 2021

Experiencing the same with "vanilla" mailgun.js in a Next.js serverless function.

@maxfi
Copy link

maxfi commented May 23, 2021

Experiencing the same with "vanilla" mailgun.js.

Me too. I've switched back to https://github.com/mailgun/mailgun-js-boland until this is resolved.

@tobimori
Copy link

tobimori commented May 23, 2021

Experiencing the same with "vanilla" mailgun.js.

Me too. I've switched back to mailgun/mailgun-js-boland until this is resolved.

Is it faster/better than SMTP? Came here to replace nodemailer SMTP transport with this.

@maxfi
Copy link

maxfi commented May 23, 2021

Is it faster/better than SMTP? Came here to replace nodemailer SMTP transport with this.

I've been using it for a while, and, although it's no longer maintained, it's been working fine for sending emails, which is all I've used it for.

From the docs:

You can also use good old SMTP to send messages. But you will have to specify all advanced sending options via MIME headers

@kentcdodds
Copy link
Author

For anyone else who only really needs to send simple emails (like me), here's what I wrote to do that with the API directly:

type MailgunMessage = {
  to: string
  from: string
  subject: string
  text: string
  html: string
}

const auth = `${Buffer.from(`api:${mailgunSendingKey}`).toString('base64')}`

function sendEmail(message: MailgunMessage) {
  return fetch(`https://api.mailgun.net/v3/${mailgunDomain}/messages`, {
    method: 'post',
    body: new URLSearchParams(message),
    headers: {
      Authorization: `Basic ${auth}`,
    },
  })
}

Docs:

Look mah, no dependencies!

@olexandr-mazepa
Copy link
Collaborator

Hello @kentcdodds
I updated the terser plugin configuration to exclude class names from the minification process.
This update should fix the issue.
The updated configuration will be used for builds start from version 3.5.7

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 a pull request may close this issue.

6 participants