Skip to content

docs(@nylas/connect): rewrite README - #95

Merged
nbarraclough merged 4 commits into
mainfrom
docs/nylas-connect-readme
Sep 1, 2026
Merged

docs(@nylas/connect): rewrite README#95
nbarraclough merged 4 commits into
mainfrom
docs/nylas-connect-readme

Conversation

@nbarraclough

@nbarraclough nbarraclough commented Sep 1, 2026

Copy link
Copy Markdown
Member

Restructures packages/nylas-connect/README.md onto the house style shared by nylas-nodejs, nylas-python, nylas-ruby and nylas-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 on ConnectResult (src/types.ts:163) — it lives on result.grantInfo (types.ts:181). Copying the snippet printed Connected: undefined with nothing to explain why. Now result.grantInfo?.email.

Structural fixes

Problem Fix
Two # H1s — the identity-provider doc was concatenated below the FAQ One H1; that content folded into Usage
Two ## Error Handling sections (old lines 103 and 325) One
Config table listed 6 of the 11 ConnectConfig options, omitting identityProviderToken, environment, defaultScopes, autoHandleCallback, logLevel All 11, verified against src/types.ts:110
No links to developer.nylas.com in 340 lines Linked per section, matching the other SDKs
Payoff of the quickstart was a console.log Ends on a real API call
No mention of reusing your own user IDs New Keep your own user IDs section covering identityProviderToken

The error section now distinguishes the 6 exported error classes (src/index.ts) from the 9 OAuthError subclasses, which are thrown by the createOAuthError factory but not exported — so they must be narrowed with error.name rather than instanceof.

Stated runtime: Node 18+ → 22+

The old README advertised Node 18+. That was not achievable: src/crypto/pkce.ts:12,34 call crypto.subtle.digest and crypto.getRandomValues on the bare global crypto, with no polyfill and no node:crypto import anywhere in src/. Node 18 did not expose the Web Crypto global unflagged. Removing that global and running the PKCE path reproduces the failure:

TypeError: Cannot read properties of undefined (reading 'getRandomValues')

22+ matches the root README. This is a documentation correction only — no engines field is added, so nothing about installation behaviour changes.

Open items not addressed here

  • X-Nylas-External-User-Id is documented in the new section but has no API reference — it, external_user_id, and idp_claims are all absent from api-docs-v3/. Worth adding to the spec separately.
  • package.json description, keywords, and homepage could all be improved (the description sells PKCE, an implementation detail, and never says email or calendar). Left alone.
  • The four broken @nylas/react Connect imports are fixed separately in fix(@nylas/react): correct Connect import paths in README #94.

🤖 Generated with Claude Code

@nbarraclough
nbarraclough requested review from a team and radenkovic as code owners September 1, 2026 14:48
@nbarraclough nbarraclough changed the title docs(@nylas/connect): rewrite README and declare supported Node range docs(@nylas/connect): rewrite README Sep 1, 2026
@nbarraclough
nbarraclough force-pushed the docs/nylas-connect-readme branch from 4713473 to c56fddc Compare September 1, 2026 14:54
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>

@AaronDDM AaronDDM left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good afternoon!

Nice cleanup overall — the structure is much better and I verified the substantive corrections against the source:

  • result.email really doesn't exist on ConnectResult (src/types.ts:163); grantInfo.email does (types.ts:181). Good catch.
  • ConnectConfig has exactly 11 options (types.ts:110-134) and the new table covers all 11 with the right defaults.
  • 9 OAuthError subclasses in errors/connect-errors.ts, 6 error classes exported from src/index.ts. The error.name guidance is correct.
  • Provider is google | 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,34 do 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.

Comment thread packages/nylas-connect/README.md Outdated
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>
@nbarraclough

Copy link
Copy Markdown
Member Author

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 result.accessToken with the /me/ syntax, and notes where an API key is still required.

2. getAuthUrl() undocumented. You're right that the rewrite dropped it. Added a Backend-only flow subsection under Custom code exchange, documenting the real { url, state, scopes } return and that it builds the URL without PKCE so your backend can exchange the code as a confidential client:

const { url, state, scopes } = await nylasConnect.getAuthUrl();
window.location.href = url;

Plus a note that callback() and getSession() play no part in that flow, since nothing is stored client-side.

One heads-up on the branch: my first push was rejected as non-fast-forward because main had been merged in here and had moved on (#78). I reset to the remote state and cherry-picked on top rather than forcing, so that merge is intact — the push went in as a fast-forward.

Ready for another look.

@nbarraclough
nbarraclough merged commit 2ab4134 into main Sep 1, 2026
6 checks passed
@nbarraclough
nbarraclough deleted the docs/nylas-connect-readme branch September 1, 2026 18:16
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 this pull request may close these issues.

2 participants