refactor(firestore-send-email): construct MailService from the named export - #3158
refactor(firestore-send-email): construct MailService from the named export#3158IzaakGough wants to merge 2 commits into
Conversation
…export The default import of @sendgrid/mail only resolved through tsc's esModuleInterop helper, so the transport broke silently whenever that compiler option changed. A named import emits a plain property access and needs no helper at all. Each transport now builds its own MailService instead of calling setApiKey on the module singleton, so two transports cannot clobber each other's key. Drops the compiled-output test that guarded the interop, which the import no longer depends on.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the SendGridTransport to use an isolated instance of MailService instead of the shared global singleton from @sendgrid/mail. This prevents configuring the API key from affecting other transports in the same process. The corresponding tests and mocks have been updated to support this instantiation change, and an obsolete test file was removed. I have no additional feedback to provide.
There was a problem hiding this comment.
Approving. Verified against @sendgrid/mail@8.1.6: the default export really is a shared singleton, separate MailService instances get separate clients, and the named import emits a plain require with no __importDefault wrapper, so the interop bug is structurally gone. No regression from dropping the ambient key either, since there's no SENDGRID_API_KEY fallback and helpers.ts always passes one.
Two non-blocking follow-ups:
- Consider keeping
tests/build-interop.test.ts. With it gone, nothing exercises the built output or the real module shape, and it passes unchanged here. - The mock returns the same object for every construction, so a revert to
sgMail.setApiKey(...)would still pass. Worth assertingsgMail.MailServicewas constructed.
The mocks returned the same spies for the singleton and for every construction, so a revert to sgMail.setApiKey passed the whole suite. Two tests now assert the transport constructs a MailService and that two transports keep separate keys without touching the singleton. Also drops `default: mail` from both mocks, unused since the transport stopped default-importing the package.
|
@CorieW I agree that having the build-interop test would be good. The release pipeline at the moment doesn't build before running the tests so if we add this back it will still fail. Do you think we should add a build step in the yaml before running the tests or maybe a script in the package.json only for this kit? |
|
Oops, sorry Izaak. Didn't mean to send that comment yet. Had to review it first, but it jumped the gun. |
3 participants
What was broken
src/nodemailer-sendgrid/index.tsimported@sendgrid/mailas a default import. That package assigns aMailServiceinstance straight tomodule.exportswith no__esModulemarker, so the import only resolved at runtime through tsc'sesModuleInterophelper. Turn that option off and the emittedrequirehas no.default, so the transport throws on construction.tscdoes not always catch it: withallowSyntheticDefaultImportson, the build is clean and the break is silent until a user's mail actually sends.What changed
The package also exports the class (
module.exports.MailService, declared asexport {MailService}). A named import compiles to a plain property access and needs no interop helper, so the emitted output no longer contains__importDefaultat all.Each transport now constructs its own
MailServicerather than callingsetApiKeyon the shared singleton. Not a bug today, the kit only ever builds transports with one key, but two transports with different keys previously clobbered each other.tests/build-interop.test.tsis removed. It existed to catch this class of break in the compiled output, and the import no longer depends on the setting it guarded.How it was verified
Clean
npm cifrom the shrinkwrap, build, 157 tests pass. Against the real@sendgrid/mailand the builtlib/: the key reaches the client, two transports keep separate keys, and the module singleton is never mutated. Rebuilt withesModuleInterop: falseplusallowSyntheticDefaultImports: true, the config that silently broke the old code, and all of that still passes.No CHANGELOG entry: there is no user-visible behaviour change. Happy to add one if you would rather record the hardening.