search

verygoodsecurity / collect-js-react

MIT License
1 stars 15 forks source link

The use of `nanoid` leads to ERR_REQUIRE_ESM errors when loading collect-js-react in node #54

Open fpapado opened 1 month ago

fpapado commented 1 month ago

Expected Behavior

The expected behavior is that @vgs/collect-js-react can be used in React trees that are consumed by a CJS node process, such as Server-Side Rendering (SSR), without crashing. This sounds quite terse, the "current behavior" section below explains it better 😅

Current Behavior

@vgs/collect-js-react is published as a dual-module package: there is a CJS version marked by the main field in package.json (dist/index.js) and an ESM version marked by the module field (dist/index.modern.js). Bundlers and node know how to resolve each one.

However, there is a quirk: nanoid is an ESM-only package, which is ok by itself. However, when collect-js-react's dist/index.js is loaded by node as CJS, the resolution eventually reaches the require('nanoid') statement. The current stable node versions do not allow synchronous requires of ESM modules, leading to the ERR_REQUIRE_ESM error at runtime.

This scenario can happen in SSR setups, where a node process is typically used to render React trees to HTML. While it is possible to run ESM-only node servers, our current setup at work is still CJS, and I believe that is a common setup elsewhere. A module graph that imports collect-js-react will crash in such scenarios.

In many cases, TypeScript can actually highlight these issues while building the library. I noticed that the moduleResolution setting in this library's tsconfig.json is node, which is the old/legacy node behavior (nowadays known as node10). If you configure "moduleResolution": "node16", you should be seeing an error about the ESM-only nanoid being imported in a CJS context at type-checking time.

Here is TypeScript's error, after tweaking the config and dependencies in this repository to use typescript@^5:

src/Fields.tsx:3:24 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("nanoid")' call instead.
  To convert this file to an ECMAScript module, add the field `"type": "module"` to '/Users/fotis/collect-js-react/package.json'.

3 import { nanoid } from 'nanoid'

Possible Solution

First idea: nanoid@^3

One possible solution is to use nanoid@^3, which is still supported, and is also a dual package (nanoid themselves recommend this approach, citing that they still support it). This might be the simplest approach, as far as I can see.

Second idea: useId, with some caveats

My other thought was to use React's native useId to support the ids for the fields. nanoid is only used to initialise and query fields, which I don't think needs a full UUID construct (but please tell me if I am wrong 🙇)

However, there are some constraints around using useId.

The first constraint, is that collect-js-react supports react 16/17/18 (according to package.json peerDependencies). useId is only available in React 18 and upward. While we could feature-detect its presence in the React exports, there would still be an issue for users of other react versions. Conditionally require()ing nanoid is also not great in this scenario, because the ERR_REQUIRE_ESM error would persist for those users.

A possible solution is to use a shim for useId, such as a global counter, for React <18. A global counter can lead to hydration errors in SSR scenarios, but the id itself is never actually rendered in the DOM on the server (because @vgs/collect-js runs only on the client, right?).

There are also some other smaller issues with useId, such as having to CSS.escape() it, because it ends up being used in document.querySelector.

I am happy to help prototype this, if you want. However, using nanoid version 3 might solve this with less complexity.

Steps to Reproduce (for bugs)

I made a minimal demo with a reproduction inside: https://github.com/fpapado/vgs-collect-js-react-esm-require-error. There are a few moving pieces, so this was simpler 😇

Context

We have been using @vgs/collect-js and wanted to use the first-party React bindings. Version 1.1.0 was working well for us, but 1.2.0 surfaced this issue.

I believe this is a breaking change between version 1.1.0 and 1.2.0, since the public-facing API of the module (the way it is packaged, and its runtime compatibility) have changed.

While we could stay on 1.1.0, we worry that we will fall behind on future feature additions and security updates, so we would like to resolve this hurdle somehow. We like VGS and have spent a while thinking about library packaging, so we thought to offer some context and help 😌

Please let me know if there is any other information that I can provide, and thank you for all the work you do on VGS!

Your Environment

filipebarcos commented 1 month ago

I'm having the same issue. We're integrating VGS for the first time, and started off with version 1.2.0.

Same thought, we could use 1.1.0, at least to unblock ourselves, but ideally this would be fixed so we can continue to update versions

flor-master commented 1 month ago

Hi @fpapado, @filipebarcos, I see the issue thanks for the detailed issue explanation, I was impressed by this :) We will release a fix as soon as possible.

flor-master commented 2 weeks ago

Hi guys, we’re replacing nanoID with a built-in JavaScript function. New version 1.3.0