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

feat: full Node.js ESM runtime support #86

Merged
merged 1 commit into from Feb 2, 2023
Merged

feat: full Node.js ESM runtime support #86

merged 1 commit into from Feb 2, 2023

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Jan 9, 2023

Squash merge commit body:

BREAKING CHANGE: We have removed the default export and replaced it with a named one:

    ```diff
    -import SanityClient from '@sanity/client'
    +import {createClient} from '@sanity/client'
    ```
We're working on a codemod for this migration.
[The migration guide outlines every breaking change and how to migrate your code](https://github.com/sanity-io/client#from-v4)

- feat: full ESM support in modern runtimes and tooling (Bun, Deno, Edge, without breaking Node.js ESM and CJS compatibility)
- feat: codebase rewritten in TypeScript, typings are generated and no longer manually maintained
- feat: make `httpRequest` on `SanityClient` configurable, intended for libraries wishing to extend the client
- feat: shipping modern syntax, reducing bundlesize

How to test

  • npm install @sanity/client@esm
  • browser (DevTools Console): const {createClient} = await import('https://esm.sh/@sanity/client@esm')
  • umd: <script src="https://unpkg.com/@sanity/client@esm"></script>

The generated typings shouldn't diverge from the original. Maybe we can use api-extractor to compare the two .d.ts files and see exactly what's changed?

Docs

Changes

  • Codebase rewritten to TypeScript and modern JS syntax to make it easier to build new features.
  • Generating types from source, no more manual type definitions.
  • Replaced @babel/cli and esbuild tooling with @sanity/pkg-utils, ensuring the generated bundles, typescript definitions and pkg.exports are following our best practices.
  • The new ESM setup brings us to 100% coverage for our runtimes target, tested with npm i @sanity/client@esm end-to-end.
  • Upgraded to get-it v8 which ships with the same ESM support and modern syntax as this PR.
  • Deprecated APIs are removed, with migration instructions in the readme. Some APIs are still marked as @deprecated, but they're actually "internals" that other core libraries we maintain rely on. Marking them @deprecated will in most IDE's create a strikethrough marking, to help surface to the third party that they're using an API that could have breaking changes in any release.
  • Refactored internals to use private class properties and closures. What remains are public APIs and internal ones that other libraries, like sanity, rely on.
  • Replaced the pkg.browser setup with the same setup get-it uses for conditionally loading dependencies that use Node APIs (process.env) or Web/Browser/Worker APIs (location.url). They generate a separate bundle for Node src/index.ts => dist/index.js and Browsers src/index.browser.ts => dist/browser.js. The previous setup with pkg.browser is only supported by bundlers like browserify and webpack. The new setup will load the browser or node optimal version of the client for you automatically if the environment supports pkg.exports. If pkg.exports isn't supported then you can fallback to manually require('@sanity/client/dist/index.browser.cjs') or import('@sanity/client/dist/index.browser.js') and there's still no bundler required.
  • The new ESM support in Node.js doesn't sacrifice CJS compatibility. Wether you require('@sanity/client') or import '@sanity/client' is up to you.
  • The ESM implementation is safe from the Dual Package Hazard unique to the Node.js runtime. This means that checks like client instanceof SanityClient works even if client is loaded by CJS while SanityClient is an ESM import.
  • sideEffects: false added to @sanity/client/package.json and get-it/package.json to enable much better tree-shaking dead code elimination in bundlers like webpack.
  • Bundlesize reductions:

Internal changes (context for the team)

  • Replaced custom build tooling with @sanity/pkg-utils, and applied its best practice for dealing with ESM interop with Node.js, bundlers and runtimes.
  • Replaced tape with vitest.
  • Tests are refactored to TypeScript to aid quality assuring the typings we generate, increasing their accuracy.
  • Replaced istanbul for codecoverage metrics to c8, as it's using the built in Node.js coverage tools and have better support for esm.
  • The migration to TypeScript used sanityClient.d.ts as point of reference, however it only contains public typings. That's why there's a liberal use of as any. Implicit any aren't allowed, they have to be explicit. This makes it easier to come back later and type them properly.
  • Because of the amount of any warnings in ESLint the CI uses --quiet to hide them. Once the any types are taken care of the intention is to set the CI to use --max-warnings 0 instead.
  • The CI is setup to report failing tests as annotations. It's as if vitest leaves a code review on your PR and requests changes on the failing tests directly. Saving you the round trip of looking at CI logs and figuring out where the code is in your IDE.
  • The vitest runs the suite in different environments (node, happy-dom/browser-like, edge-runtime) with different pkg.exports conditions to try and cover as much ground as possible.
  • The deno runtime test is refactored to use import_map.json without a deno.lock file since our renovatebot setup doesn't support deno. The new setup have a deno.yml action that, on commits pushed to main that touches package.json, will update the import_map.json to use the same semver range as the client dependencies.
  • There's a Node.js runtime suite (npm run test:node-runtimes) that uses the built-in test runner introduced in v18. This suite tests that Node.js is able to import the client in both its CJS and ESM runtime without the help of a bundler.
  • The CI is setup to automatically run npx prettier --write . if commits are pushed to main that forgot to run it. Especially useful if renovatebot updates prettier to a new version with formatting changes, so that the next person who wants to change 1 line of code in a PR don't end up with prettier creating tons of unrelated changes on their PR.
  • And finally, there's a lock.yml GitHub action that automatically locks issues and PRs once they're closed. We don't get notifications if someone posts a comment on a closed thread, and so by locking them we nudge people to create a new issue. Enabling us to follow up and triage promptly.

Next steps

  • Work through the any usage in the client, and get-it.
  • Remove extract.rules from package.config.ts and use defaults, correct incorrect tags.
  • Either reuse @sanity/types, or move them here so we have a single source of truth for things like SanityDocument and SanityImageAsset.
  • Look into if @sanity/pkg-utils can handle the checks we're doing in npm run test:node-runtimes and npm test exports.test.ts as part of its package.json validation.
  • Update our docs as we're still saying that the Client requires Promise to be polyfilled, now that we use native ESM as the baseline maybe we should explain that instead?
  • Lazyload @sanity/eventsource instead of always bundling it , we do this in @sanity/preview-kit already for bundlesize reasons. And since it's only used with client.listen, which is async, we can do this without introducing a breaking change.

@stipsan stipsan force-pushed the modernize branch 27 times, most recently from 011ac74 to 2eec069 Compare January 10, 2023 20:53
@stipsan stipsan requested a review from bjoerge January 10, 2023 23:51
@stipsan stipsan marked this pull request as ready for review January 10, 2023 23:51
@stipsan stipsan requested a review from rexxars January 10, 2023 23:52
@dburles

This comment was marked as resolved.

@stipsan

This comment was marked as resolved.

@dburles

This comment was marked as resolved.

@stipsan

This comment was marked as resolved.

@stipsan

This comment was marked as resolved.

@sanity-io sanity-io locked as resolved and limited conversation to collaborators Jan 17, 2023
@rexxars
Copy link
Member

rexxars commented Jan 17, 2023

I'm doing a check in the Sanity monorepo to see that the new client works as expected there (and migrating some code needed to make it work along the way). Will post any blockers here.

@rexxars

This comment was marked as resolved.

@rexxars

This comment was marked as resolved.

@stipsan
Copy link
Member Author

stipsan commented Jan 18, 2023

Hmm, that is new and breaking behavior - previously you could either use it as a function or call it with new (internally the function variant would instantiate a new class based on the config, if passed). If this behavior is changed, we need to explicitly call it out in the breaking changes, and we also need to update the codemod to alter any new Client calls to be the function variant.

In terms of the naming, it is a more complicated codemod to make, as you have to make sure createClient is not declared anywhere else, and update all the references to the function. I don't personally see any reason to go the extra mile on this - I might even prefer it to be alias to createSanityClient in my own code for readability.

Ok it sounds like it'll be a painful migration. The tricky bit here is that httpRequest is different in index.ts and index.browser.ts and is now the first constructor argument in SanityClient. In order to make the following codemod "just work":

-import SanityClient from '@sanity/client'
+import {SanityClient} from '@sanity/client'

const client = new SanityClient({projectId, ...etc})

Then the index files will have to re-export wrappers:

import envMiddleware from './http/nodeMiddleware'
import {defineHttpRequest} from './http/request'
import {SanityClient as BaseSanityClient} from './SanityClient'

// Set the http client to use for requests, and its environment specific middleware
const httpRequest = defineHttpRequest(envMiddleware)

export class SanityClient extends BaseSanityClient {
  constructor(config: ClientConfig) {
    super(httpRequest, config)
  }
}

export const createClient = (config: ClientConfig) => new BaseSanityClient(httpRequest, config)

Thoughts @rexxars? Yeah we should probably have the core classes change the constructor signature to match so it's consistent, but I'd like to hear your thoughts on the public API 🤔

@stipsan stipsan force-pushed the modernize branch 2 times, most recently from 0bc4bc9 to 0cc2ac6 Compare January 18, 2023 13:49
@rexxars
Copy link
Member

rexxars commented Jan 19, 2023

Thoughts @rexxars? Yeah we should probably have the core classes change the constructor signature to match so it's consistent, but I'd like to hear your thoughts on the public API 🤔

Hmm, maybe we should just pull the plug on the constructor signature. I'll see if I can find time to update the codemod tomorrow 👍

@rexxars

This comment was marked as resolved.

@stipsan

This comment was marked as resolved.

@stipsan stipsan force-pushed the modernize branch 3 times, most recently from c0e1810 to 6a95d12 Compare January 26, 2023 01:13
@stipsan stipsan force-pushed the modernize branch 4 times, most recently from 8883151 to 42f9fd5 Compare February 1, 2023 21:33
BREAKING CHANGE: We have removed the default export and replaced it with a named one:

```diff
-import SanityClient from '@sanity/client'
+import {createClient} from '@sanity/client'
```

[The migration guide outlines every breaking change and how to migrate your code](https://github.com/sanity-io/client#from-v4)

- feat: full ESM support in modern runtimes and tooling (Bun, Deno, Edge, without breaking Node.js ESM and CJS compatibility)
- feat: codebase rewritten in TypeScript, typings are generated and no longer manually maintained
- feat: make `httpRequest` on `SanityClient` configurable, intended for libraries wishing to extend the client
- feat: shipping modern syntax, reducing bundlesize

Co-Authored-By: Knut Melvær <knut@sanity.io>
Co-Authored-By: Espen Hovlandsdal <espen@hovlandsdal.com>
Co-Authored-By: Bjørge Næss <876086+bjoerge@users.noreply.github.com>
Copy link
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

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

Have tested this extensively in the studio codebase - things seems to be working as expected! Will try to get a codemod ready for (constructor => factory method) tomorrow!

@stipsan stipsan merged commit bd9b247 into main Feb 2, 2023
@stipsan stipsan deleted the modernize branch February 2, 2023 12:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants