-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
011ac74
to
2eec069
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Ok it sounds like it'll be a painful migration. The tricky bit here is that -import SanityClient from '@sanity/client'
+import {SanityClient} from '@sanity/client'
const client = new SanityClient({projectId, ...etc}) Then the 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 |
0bc4bc9
to
0cc2ac6
Compare
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 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
c0e1810
to
6a95d12
Compare
8883151
to
42f9fd5
Compare
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>
There was a problem hiding this 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!
Squash merge commit body:
How to test
npm install @sanity/client@esm
const {createClient} = await import('https://esm.sh/@sanity/client@esm')
<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
@babel/cli
andesbuild
tooling with@sanity/pkg-utils
, ensuring the generated bundles, typescript definitions andpkg.exports
are following our best practices.npm i @sanity/client@esm
end-to-end.get-it
v8
which ships with the same ESM support and modern syntax as this PR.@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.sanity
, rely on.pkg.browser
setup with the same setupget-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 Nodesrc/index.ts => dist/index.js
and Browserssrc/index.browser.ts => dist/browser.js
. The previous setup withpkg.browser
is only supported by bundlers likebrowserify
andwebpack
. The new setup will load the browser or node optimal version of the client for you automatically if the environment supportspkg.exports
. Ifpkg.exports
isn't supported then you can fallback to manuallyrequire('@sanity/client/dist/index.browser.cjs')
orimport('@sanity/client/dist/index.browser.js')
and there's still no bundler required.require('@sanity/client')
orimport '@sanity/client'
is up to you.Dual Package Hazard
unique to the Node.js runtime. This means that checks likeclient instanceof SanityClient
works even ifclient
is loaded by CJS whileSanityClient
is an ESM import.sideEffects: false
added to@sanity/client/package.json
andget-it/package.json
to enable much better tree-shaking dead code elimination in bundlers like webpack.Internal changes (context for the team)
@sanity/pkg-utils
, and applied its best practice for dealing with ESM interop with Node.js, bundlers and runtimes.tape
withvitest
.istanbul
for codecoverage metrics toc8
, as it's using the built in Node.js coverage tools and have better support for esm.as any
. Implicitany
aren't allowed, they have to be explicit. This makes it easier to come back later and type them properly.any
warnings in ESLint the CI uses--quiet
to hide them. Once theany
types are taken care of the intention is to set the CI to use--max-warnings 0
instead.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.vitest
runs the suite in different environments (node
,happy-dom
/browser-like,edge-runtime
) with differentpkg.exports
conditions to try and cover as much ground as possible.import_map.json
without adeno.lock
file since our renovatebot setup doesn't supportdeno
. The new setup have adeno.yml
action that, on commits pushed to main that touchespackage.json
, will update theimport_map.json
to use the same semver range as the clientdependencies
.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.npx prettier --write .
if commits are pushed tomain
that forgot to run it. Especially useful if renovatebot updatesprettier
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 withprettier
creating tons of unrelated changes on their PR.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
any
usage in the client, andget-it
.extract.rules
frompackage.config.ts
and use defaults, correct incorrect tags.@sanity/types
, or move them here so we have a single source of truth for things likeSanityDocument
andSanityImageAsset
.@sanity/pkg-utils
can handle the checks we're doing innpm run test:node-runtimes
andnpm test exports.test.ts
as part of itspackage.json
validation.Promise
to be polyfilled, now that we use native ESM as the baseline maybe we should explain that instead?@sanity/eventsource
instead of always bundling it , we do this in@sanity/preview-kit
already for bundlesize reasons. And since it's only used withclient.listen
, which is async, we can do this without introducing a breaking change.