fix: make types compile under TypeScript 7.0 - #1458
Open
FelipeLahti wants to merge 2 commits into
Open
FelipeLahti wants to merge 2 commits into
FelipeLahti wants to merge 2 commits into
Conversation
Both `@sendgrid/client` and `@sendgrid/mail` ship .d.ts files that
mix `export = instance` with `export {Class}` and silence the
diagnostic with `// @ts-ignore`. Pre-7.0, downstream consumers paid
no cost for this since named imports of the class still resolved.
TS 7.0 (the Go-port `tsgo` beta) tightens the rule and surfaces
TS2616 in every consumer that imports `Client` or `MailService`:
error TS2616: 'Client' can only be imported by using
'import Client = require("@sendgrid/client")' or a default import.
The fix mirrors the runtime: `module.exports = new Client();
module.exports.Client = Client;`. Modeling that as a `Client`
instance with a `Client: typeof Client` instance member lets TS
expose the class through `client.Client` AND through the named
import `import { Client } from "@sendgrid/client"` without any
escape hatches. Same pattern for MailService. Existing TS test
fixtures (incl. `new Client()` and `setClient(client: Client)`)
keep passing under both tsc 5.9 and tsgo 7.0 unchanged.
Also bring tsconfig.json up to TS 7.0 minimums so the test fixtures
type-check under tsgo: drop the removed `baseUrl`, add the now-
required leading `./` to path mappings, and declare `types: ["node"]`
explicitly (auto-load of all @types is gone in 7.0).
Verified with tsc@5.9.3 and @typescript/native-preview@7.0.0-dev.
Frown00
reviewed
Aug 19, 2026
| export = client | ||
|
|
||
| export {Client}; | ||
| export = client; |
There was a problem hiding this comment.
I wanted to create an MR for the exact same reason. From my testing, this is the only change needed. No other changes are required.
export default client;Of course, I'm not completely sure if this won't break anything else
Attaching `Client` as an instance member made it a value-only export, so `function f(c: Client)` stopped type-checking, and it could not carry the type-only `MailDataRequired`, `ClientResponse` and `ResponseError` re-exports at all. Declaring an ambient namespace merged with the `export =` target models `module.exports` exactly — the singleton's methods plus the class attached in index.js — and keeps both meanings of `Client` and `MailService` importable by name. No `@ts-ignore`, no runtime change. Fixtures now cover `client.Client`, `Client` in type position, and the types re-exported from the package root; each of those failed to compile under the previous shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1454
@sendgrid/clientand@sendgrid/mailmixexport = instancewithexport {Class}and silence the diagnostic with// @ts-ignore. TypeScript 7.0 (the Go-porttsgobeta) tightens the rule and surfaces it in every consumer:The two exports were never reconcilable as written, because
module.exportsis a singleton instance with a class attached to it (index.jsdoesmodule.exports = client; module.exports.Client = Client), and adeclare constcannot carry named exports.So model that shape directly: declare an ambient namespace that merges with the
export =target. The namespace holds the singleton's methods plusexport {Client}, which keeps both meanings of the name — the class as a value and as an instance type — importable by name, with no@ts-ignore.Same shape applied to
MailServiceinpackages/mail/src/mail.d.ts, where the namespace also carries theMailDataRequired,ClientResponseandResponseErrorre-exports.tsconfig.jsonis also bumped to TS 7.0 minimums sotest/typescript/*.tskeeps type-checking undertsgo:baseUrlremoved, leading./added to path mappings,types: ["node"]declared explicitly. Without thistsgobails on the config before it type-checks anything.No runtime change.
Verification
Both compilers pass:
I also checked the consumer-visible surface end to end — a separate project with
@sendgrid/mail@8.1.6and@sendgrid/client@8.1.6installed from npm and only the.d.tsfiles swapped, so resolution goes through the publishedindex.d.tsrather than this repo'spathsmapping:mainimport sg = require("@sendgrid/client"),new sg.Client()import { Client }→new Client()import { Client }→c: Client(type position)import { MailDataRequired, ClientResponse, ResponseError }import * as sg from "@sendgrid/mail"import sg from "@sendgrid/mail"(esModuleInterop)Nothing that compiles against
mainstops compiling, andsg.Client— which the runtime has always provided but the types denied — now works.The same matrix passes under
tsgowithmodule: nodenext(CommonJS consumer). Againstmain,tsgorejects every row withTS2595, becausemail.d.tsitself importsClientby name.I also tried the one-line alternative suggested in review,
export default client;in place ofexport = client. It fails the repo's owntest/typescriptfixtures under both compilers (27×TS2339: Property 'setApiKey' does not exist on type 'typeof import(...)'), becauseimport sg = require(...)andimport * as sgthen resolve to the module namespace, whose methods live on a.defaultthat does not exist at runtime. It also letsimport sg from "@sendgrid/mail"compile withoutesModuleInterop, which crashes at runtime withCannot read properties of undefined (reading 'setApiKey'). The only pattern it satisfies is a default import in a CommonJS project withesModuleInteropon.Known limitation: ESM namespace and named imports
In a native ESM consumer (
"type": "module",module: nodenext),import * as sg from "@sendgrid/mail"; sg.setApiKey()andimport { setApiKey } from "@sendgrid/mail"now type-check but fail at runtime. Node only exposesmodule.exports.MailServiceas a named export; the instance methods live on the prototype and are invisible to its CommonJS export detection.mainrejected these at compile time because the singleton carried no namespace at all. This is the same gap everyexport =package with a namespace merge has, and TypeScript offers no way to model it (aconstcannot merge with a namespace). ESM consumers should use the default import,import sg from "@sendgrid/mail", which compiles and runs under bothtscandtsgo.import { MailService }andimport { Client }are fine in ESM, since those are realmodule.exportsproperties.Fixtures
Three cases the existing fixtures did not reach are now covered, each of which fails against
main:new Client.Client()off the module (test/typescript/client.ts) —TS2339onmainClientin type position (test/typescript/mail.ts) — the only usage the oldexport {Client}protectedtest/typescript/mail.ts)Checklist