docs(@nylas/connect): rewrite README - #95
Conversation
4713473 to
c56fddc
Compare
Restructures the README onto the house style shared by nylas-nodejs, nylas-python, nylas-ruby and nylas-java, and fixes several claims that did not match the source: - The first usage example read `result.email`, which does not exist on `ConnectResult`. The address is on `result.grantInfo`. - The config table listed 6 of the 11 `ConnectConfig` options, omitting `identityProviderToken` among others. All 11 are now documented. - The identity-provider section was appended below the FAQ under a second `# H1`, with a second `## Error Handling`. Both are folded in. - The error section now distinguishes the 6 exported error classes from the 9 `OAuthError` subclasses, which are thrown but not exported and so must be narrowed with `error.name`. - The stated runtime was "Node.js 18+", which src/crypto/pkce.ts cannot honour: it calls `crypto.subtle` and `crypto.getRandomValues` on the bare global, with no polyfill and no `node:crypto` import. Now 22+, matching the root README. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c56fddc to
0a65d7d
Compare
AaronDDM
left a comment
There was a problem hiding this comment.
Good afternoon!
Nice cleanup overall — the structure is much better and I verified the substantive corrections against the source:
result.emailreally doesn't exist onConnectResult(src/types.ts:163);grantInfo.emaildoes (types.ts:181). Good catch.ConnectConfighas exactly 11 options (types.ts:110-134) and the new table covers all 11 with the right defaults.- 9
OAuthErrorsubclasses inerrors/connect-errors.ts, 6 error classes exported fromsrc/index.ts. Theerror.nameguidance is correct. Providerisgoogle | microsoft | imap | icloud, zero runtime deps, ESM-only, and the demo (index.html,callback.html,auth-instance.js,pnpm dev) all check out.- The Node 22 correction is justified —
crypto/pkce.ts:12,34do use the bare global.
One blocker and one gap:
1. The quickstart puts a Nylas API key in browser code. See the inline comment on line 95.
2. getAuthUrl() is now undocumented. It's public (connect-client.ts:437) and the old README covered it under "Backend-Only Flow". Nothing in the new README mentions it. Either add it back under Custom code exchange or link the docs page that covers it, otherwise there's no way to discover it from here.
Happy to re-review as soon as the API key snippet is fixed.
Two fixes from @AaronDDM's review: 1. The quickstart authorized a browser fetch with `process.env.NYLAS_API_KEY`. `connect({ method: "popup" })` is browser-only, so that either resolves to nothing or gets inlined into the shipped bundle. It now uses the `accessToken` that `connect()` returns, and addresses the mailbox as `/v3/grants/me/`, which is the documented pattern for access-token auth. Adds a note on when an API key is still required and where it belongs. 2. `getAuthUrl()` is public (`connect-client.ts:437`) and was covered by the old README's "Backend-Only Flow", but the rewrite dropped it. Adds a "Backend-only flow" subsection under Custom code exchange documenting its `{ url, state, scopes }` return and that it builds the URL without PKCE for a confidential server-side exchange. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review — both points were fair. Addressed in 7c2e567. 1. API key in browser code. Fixed; details in the inline thread. Short version: now uses 2. const { url, state, scopes } = await nylasConnect.getAuthUrl();
window.location.href = url;Plus a note that One heads-up on the branch: my first push was rejected as non-fast-forward because Ready for another look. |
Restructures
packages/nylas-connect/README.mdonto the house style shared bynylas-nodejs,nylas-python,nylas-rubyandnylas-java, and corrects claims that did not match the source.README-only. No source or package metadata changes.
Broken sample
The very first usage example read the mailbox address from
result.email. That property does not exist onConnectResult(src/types.ts:163) — it lives onresult.grantInfo(types.ts:181). Copying the snippet printedConnected: undefinedwith nothing to explain why. Nowresult.grantInfo?.email.Structural fixes
# H1s — the identity-provider doc was concatenated below the FAQ## Error Handlingsections (old lines 103 and 325)ConnectConfigoptions, omittingidentityProviderToken,environment,defaultScopes,autoHandleCallback,logLevelsrc/types.ts:110console.logidentityProviderTokenThe error section now distinguishes the 6 exported error classes (
src/index.ts) from the 9OAuthErrorsubclasses, which are thrown by thecreateOAuthErrorfactory but not exported — so they must be narrowed witherror.namerather thaninstanceof.Stated runtime: Node 18+ → 22+
The old README advertised Node 18+. That was not achievable:
src/crypto/pkce.ts:12,34callcrypto.subtle.digestandcrypto.getRandomValueson the bare globalcrypto, with no polyfill and nonode:cryptoimport anywhere insrc/. Node 18 did not expose the Web Crypto global unflagged. Removing that global and running the PKCE path reproduces the failure:22+ matches the root README. This is a documentation correction only — no
enginesfield is added, so nothing about installation behaviour changes.Open items not addressed here
X-Nylas-External-User-Idis documented in the new section but has no API reference — it,external_user_id, andidp_claimsare all absent fromapi-docs-v3/. Worth adding to the spec separately.package.jsondescription,keywords, andhomepagecould all be improved (the description sells PKCE, an implementation detail, and never says email or calendar). Left alone.@nylas/reactConnect imports are fixed separately in fix(@nylas/react): correct Connect import paths in README #94.🤖 Generated with Claude Code