chaijs / chai

BDD / TDD assertion framework for node.js and the browser that can be paired with any testing framework.
https://chaijs.github.io
MIT License
8.1k stars 695 forks source link

ERR_REQUIRE_ESM in v5 #1561

Closed ddolcimascolo closed 8 months ago

ddolcimascolo commented 8 months ago

Hi there,

I don't have all the details, as I don't have access to my workstation right now, but our CI is failing with this error for all our projects using chai when trying an upgrade to chai@5. These projects are commonjs modules using require('chai') everywhere in tests, running under Node 20.

Is chai now ESM-only?

I can gather more details if required, feel free to ask.

Thanks, David

43081j commented 8 months ago

It is esm only in 5.x

You can still use a dynamic import to import it in node at least afaik, but should probably just stick to 4.x until you can migrate to esm (feature set shouldn't be much different)

s100 commented 8 months ago

Probably should have removed all the references to require('chai') from the documentation as part of this change!

43081j commented 8 months ago

I agree, I wasn't aware a 5.0 was being published today. @koddsson if you're already digging around the repo, maybe you could update the docs too?

ddolcimascolo commented 8 months ago

OK.

That's clearly sad news and something I personally don't understand considering the plethora of build tools (from the top of my mind https://github.com/egoist/tsup and https://github.com/unjs/unbuild at least) that can double-build at near 0 cost... Lots of very large projects are still commonjs, Node still has first class support for commonjs, etc.

Anyway, thanks for confirming and long live to Chai 👍

Cheers, David

43081j commented 8 months ago

They don't build at zero cost, nothing does. Inevitably, you need to publish a CJS entry point and an ESM entry point.

On top of that, to work in browsers, you need to purge cjs entirely from the tree - this means publishing an entire cjs version and an entire esm version (no wrappers).

There's so many reasons to do this, that have been discussed.

People on very old node can continue to use 4.x, the feature set is near enough the same.

koresar commented 8 months ago

chai could have waited until Node 22 released. Because it's going to get the support of interoperability between CJS and ESM at runtime. (I ranted about it here.)

Otherwise, chai v5 is going to break a lot of CI/CD pipelines out there.

gabririgo commented 8 months ago

+1, we use import { expect} from "chai" and our build fails on the PR upgrading chai with error

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/runner/work/v3-contracts/v3-contracts/node_modules/chai/chai.js from /home/runner/work/v3-contracts/v3-contracts/test/core/RigoblockPool.Basetoken.spec.ts not supported.
Instead change the require of chai.js in /home/runner/work/v3-contracts/v3-contracts/test/core/RigoblockPool.Basetoken.spec.ts to a dynamic import() which is available in all CommonJS modules.
koddsson commented 8 months ago

+1, we use import { expect} from "chai" and our build fails on the PR upgrading chai with error

Do you have a minimal reproduction for this?

keithamus commented 8 months ago

To clarify some points:

To talk about the motivation for this change, especially from the lens of the ecosystem of available build tools: there is a complexity trade off for libraries like Chai to support each of these build tools, and it becomes somewhat of an N*M problem. Standards allow us to target a single syntax to reach the broadest support, and will be an inevitable shift in the JS ecosystem, it is just a matter of "when". Version 5 is chai's "when". Certainly if we are the first to support ESM it would cause undue friction, but if we were last it would also cause undue friction. Packages like @esm-bundle/chai already exist which demonstrate the friction is there today. There is no time we could have made this change that would satisfy all users. The Chai team is very confident we've made the right decision so support ESM only, and Chai >5 will continue to support only ESM (or whatever module system the EcmaScript standardises) indefinitely.

koresar commented 8 months ago

Any plans to do at least security fixes to Chai v4?

adrian-afl commented 8 months ago

I'm having the same issue now. Have to stay on version 4. At least being a dev dependency for testing it doesn't affect the production security if fixes are not backported to v4. But some strict companies will have a very hard time if any vulnerability appears in chai 4 or its dependencies :) Generally this is a signal to switch to something else, I think jest expect part is pretty nice.

Why did this change happen anyway? I feel like this is some misconfiguration issue and can be resolved.

@keithamus are you open for a PR that enables double build for v5?

Alternatively, I can't wait for a fork that resolves this and actually does double build.

43081j commented 8 months ago

Generally this is a signal to switch to something else

why do you think this?

v5 is ESM only, following the standard module system node and browsers are aligning on going forward

v4 still exists for those who, for whatever reason, can't use dynamic import or can't migrate to ESM.

i'm sure in terms of security, we could continue updating a v4 branch meanwhile. needs some discussion but that seems to be the sole reason we need to keep 4.x around, since we're not changing feature set.

I feel like this is some misconfiguration issue and can be resolved

no, it was an active decision to move towards using the standard module system all platforms are aligning to. it isn't a misconfiguration

it would be good to understand what your concerns drill down to, as there's absolutely no reason to be moving off chai just because you can't yet upgrade to 5.x.

adrian-afl commented 8 months ago

why do you think this?

It doesn't work with ts-node, a big point I think for me, for example, I run mocha with ts-node/register, this stops working now if chai is on v5. If there is a solution to that I'm all ears, I could really use some help if it can be resolved.

43081j commented 8 months ago

why do you think this?

It doesn't work with ts-node, a big point I think for me, for example, I run mocha with ts-node/register, this stops working now if chai is on v5. If there is a solution to that I'm all ears, I could really use some help if it can be resolved.

presumably your project is commonjs? if that's the case, you'd have to use a dynamic import to import chai 5.x. but in those cases, i'd just stick to 4.x until you can migrate your own project to use ESM.

if you can show a reproduction somewhere, i can take a look.

in ESM projects, mocha needs a loader instead of a require hook ("loader": "ts-node/esm").

i'm happy to help as it is important for all of us and many other packages to move the ecosystem forward to a standardised module system like this. i do understand it isn't a quick job to move to ESM, though, so i have no doubt we will still be supporting 4.x in some way.

gabririgo commented 8 months ago

+1, we use import { expect} from "chai" and our build fails on the PR upgrading chai with error

Do you have a minimal reproduction for this?

you can find a reproduction here: https://github.com/RigoBlock/v3-contracts/pull/413

And the error returned:

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/runner/work/v3-contracts/v3-contracts/node_modules/chai/chai.js from /home/runner/work/v3-contracts/v3-contracts/test/core/RigoblockPool.Basetoken.spec.ts not supported.
Instead change the require of chai.js in /home/runner/work/v3-contracts/v3-contracts/test/core/RigoblockPool.Basetoken.spec.ts to a dynamic import() which is available in all CommonJS modules.
    at Object.require.extensions.<computed> [as .js] (/home/runner/work/v3-contracts/v3-contracts/node_modules/ts-node/dist/index.js:851:20)
    at Object.<anonymous> (/home/runner/work/v3-contracts/v3-contracts/test/core/RigoblockPool.Basetoken.spec.ts:26:16)
    at Module.m._compile (/home/runner/work/v3-contracts/v3-contracts/node_modules/ts-node/dist/index.js:857:29)
    at Object.require.extensions.<computed> [as .ts] (/home/runner/work/v3-contracts/v3-contracts/node_modules/ts-node/dist/index.js:859:16)
    at /home/runner/work/v3-contracts/v3-contracts/node_modules/mocha/lib/mocha.js:4[14](https://github.com/RigoBlock/v3-contracts/actions/runs/7353357735/job/20019232823?pr=413#step:7:15):36
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/runner/work/v3-contracts/v3-contracts/node_modules/mocha/lib/mocha.js:411:14)
    at Mocha.run (/home/runner/work/v3-contracts/v3-contracts/node_modules/mocha/lib/mocha.js:972:10)
    at testFailures (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/builtin-tasks/test.js:102:[15](https://github.com/RigoBlock/v3-contracts/actions/runs/7353357735/job/20019232823?pr=413#step:7:16))
    at new Promise (<anonymous>)
    at SimpleTaskDefinition.action (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/builtin-tasks/test.js:101:32)
    at async Environment._runTaskDefinition (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/core/runtime-environment.js:228:[20](https://github.com/RigoBlock/v3-contracts/actions/runs/7353357735/job/20019232823?pr=413#step:7:21))
    at async Environment.run (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/core/runtime-environment.js:91:24)
    at async SimpleTaskDefinition.action (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/builtin-tasks/test.js:127:26)
    at async Environment._runTaskDefinition (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/core/runtime-environment.js:[22](https://github.com/RigoBlock/v3-contracts/actions/runs/7353357735/job/20019232823?pr=413#step:7:23)8:20)
    at async Environment._runTaskDefinition (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/core/runtime-environment.js:228:20)
    at async Environment.run (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/core/runtime-environment.js:91:[24](https://github.com/RigoBlock/v3-contracts/actions/runs/7353357735/job/20019232823?pr=413#step:7:25))
    at async main (/home/runner/work/v3-contracts/v3-contracts/node_modules/hardhat/internal/cli/cli.js:201:13) {
  code: 'ERR_REQUIRE_ESM'
}
koddsson commented 8 months ago

you can find a reproduction here: RigoBlock/v3-contracts#413

Not really a minimal reproduction 😅

I can't install the dependencies and get it up and running. A minimal reproduction should be the least amount of code and build setup in order to reproduce the error in question.

As far as I can tell with the information given this seems to be some sort of configuration issue in TypeScript? The code is using native JavaScript modules but maybe the tsconfig.json is set to output CommonJS? The quick solve here would be to dynamically import chai as that should work in a CommonJS environment.

Might be worth looking into your TypeScript setup but I don't think this is a issue with Chai.

43081j commented 8 months ago

you can find a reproduction here: RigoBlock/v3-contracts#413

it is because the package is a commonjs package (i.e. tsconfig's module is "commonjs" and your package.json doesn't have "type": "module").

your sources are ES modules, but the build output isn't. so when you execute it, it will error.

i'd recommend you stick to 4.x for now until you can migrate to ESM ("module": "nodenext" in tsconfig)

jonkoops commented 8 months ago

But some strict companies will have a very hard time if any vulnerability appears in chai 4 or its dependencies :)

If these companies find it so important they can volunteer devs and money to maintain it. Open source projects have no reason to maintain problematic code just because someone else that doesn't contribute back deems it 'important' to them.

jonkoops commented 8 months ago

It doesn't work with ts-node, a big point I think for me, for example, I run mocha with ts-node/register, this stops working now if chai is on v5. If there is a solution to that I'm all ears, I could really use some help if it can be resolved.

By default ts-node will transform ESM syntax to CommonJS. If you read the documentation you will find you can opt-in to ESM.

adrian-afl commented 8 months ago

It doesn't work with ts-node, a big point I think for me, for example, I run mocha with ts-node/register, this stops working now if chai is on v5. If there is a solution to that I'm all ears, I could really use some help if it can be resolved.

By default ts-node will transform ESM syntax to CommonJS. If you read the documentation you will find you can opt-in to ESM.

I will try that, thanks

jonkoops commented 8 months ago

@ddolcimascolo I believe you can close this issue, as the release notes answer your question:

Chai now only supports EcmaScript Modules (ESM). This means your tests will need to either have import {...} from 'chai' or import('chai'). require('chai') will cause failures in nodejs. If you're using ESM and seeing failures, it may be due to a bundler or transpiler which is incorrectly converting import statements into require calls.

43081j commented 8 months ago

you are right, i'll close this.

for anyone bumping into this error

As of 5.0.0, chai only ships with ES modules going forward (no longer commonjs).

For those of you with ES module packages

Everything should work as you'd expect. You're probably in this category if:

For those of you with CJS packages

You're in this category if you don't follow any of the previous structures, and/or you use typescript with "module": "commonjs".

In this case, you have a couple of options:

If you can't use a bundler at that point in your project, it is probably best to stay on chai 4.x until you move to ES modules one day (if ever).

ts-node

If you're using ts-node, it has support for ES modules and more information here.

Keep in mind, you would have to migrate your own project to use ES modules to use this. So please just refer to the previous 2 sections above (CJS, ESM) for whichever you've chosen to use right now.

notes

Migrating to ESM is no simple task, so i'm sure many of us will still be stuck with commonjs for the near future at least.

In those cases, my personal opinion (not speaking for chai), is that i'd use 4.x with the aim of one day performing the ESM migration.

the feature sets between 4.x and 5.x are near identical. it is perfectly fine to stay on 4.x for now

discussion still needs to happen around security fixes etc, but i'm sure we won't be leaving those of you on 4.x still behind.

tiec7 commented 8 months ago

Sorry guys but I'm new in chai and generally in testing. I followed a super-simple tutorial with selenium, JS, and chai. so I have this package.json

{
  "name": "firsttest",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "chai": "*",
    "mocha": "*",
    "selenium-webdriver": "^4.16.0"
  }
}

and this firstTest.js

const { Builder, By, Key } = require('selenium-webdriver')

var assert = require('chai').assert

async function example() {
  // launch the browser
  let driver = await new Builder().forBrowser('chrome').build()

  // navigate to app
  await driver.get('https://lambdatest.github.io/sample-todo-app/')

  // add a todo
  await driver.findElement(By.id('sampletodotext')).sendKeys('learn selenium', Key.RETURN)

  // assert
  let todoText = await driver.findElement(By.xpath('//li[last()]')).getText()

  assert.strictEqual(todoText, 'learn selenium')

  // close the browser
  await driver.quit()
}

example()

And with the new chai, this does not work. It prints this error:

Error [ERR_REQUIRE_ESM]: require() of ES Module E:\<folders>\Selenium\FirstTest\node_modules\chai\chai.js from E:\<folders>\Selenium\FirstTest\tests\firstTest.js not supported.
Instead change the require of chai.js in E:\<folders>\Selenium\FirstTest\tests\firstTest.js to a dynamic import() which 
is available in all CommonJS modules.
    at Object.<anonymous> (E:\<folders>\Selenium\FirstTest\tests\firstTest.js:4:14) {
  code: 'ERR_REQUIRE_ESM'
}

If I try to switch to import chai from 'chai' and put "type: module" and switch code to

const { Builder, By, Key } = require('selenium-webdriver')

import chai from 'chai'

async function example() {
  // launch the browser
  let driver = await new Builder().forBrowser('chrome').build()

  // navigate to app
  await driver.get('https://lambdatest.github.io/sample-todo-app/')

  // add a todo
  await driver.findElement(By.id('sampletodotext')).sendKeys('learn selenium', Key.RETURN)

  // assert
  let todoText = await driver.findElement(By.xpath('//li[last()]')).getText()

  assert.strictEqual(todoText, 'learn selenium')

  chai.expect('#sampletodotext').not.to.contain('hello')

  // close the browser
  await driver.quit()
}

example()

in package.json, it throws an error saying that

import chai from 'chai'
       ^^^^
SyntaxError: The requested module 'chai' does not provide an export named 'default'

moreover, there's no way of using .dom selector as described in the main page.

Am I missing something? Because to a beginner this seems to be a broken package.

s100 commented 8 months ago

You should avoid using wildcard dependencies versions like "chai": "*". If the tutorial told you to do that then the tutorial is giving you bad advice and it should be corrected to say something like "chai": "^4.3.10".

The reason for this is exactly what you're discovering: a new major version of chai can contain breaking changes.

adrian-afl commented 8 months ago

SyntaxError: The requested module 'chai' does not provide an export named 'default'

Can you try to do import * as chai from 'chai'

43081j commented 8 months ago

you can probably also import the webdriver consts fwiw, import {..} from 'selenium-webdriver'

and yes, adrian is correct. you need to import *

tiec7 commented 8 months ago

you can probably also import the webdriver consts fwiw, import {..} from 'selenium-webdriver'

and yes, adrian is correct. you need to import *

thanks for your answer, and thanks to @adrian-afl and @s100 too. The tutorial in fact said to use * , so thanks for the tip;

I removed from package.json both mocha and chai, did npm install (to remove them), then from command line npm install mocha npm install chai

then, I added "type: module", so now the package.json has

"dependencies": {
    "chai": "^5.0.0",
    "mocha": "^10.2.0",
    "selenium-webdriver": "^4.16.0"
  }

Then, in test file i changed to

import { Builder, By, Key } from 'selenium-webdriver'
import { assert, expect } from 'chai'

and now it perfectly works. I also learnt a new way to import, so thanks for the patience in reading my extra-big post :) Hope it can help new users to dive into chai!

tiec7 commented 8 months ago

wait: what about should ?

I tried to import { should } from 'chai' , but now if I do

let todoText = await driver.findElement(By.xpath('//li[last()]')).getText()    // contains 'learn selenium'

todoText.should.equal('learn selenium')

should does not work, and the should keyword in the import is grayed out.

How this works now?

43081j commented 8 months ago

how did you use it before?

i suspect you need to do something like this:

import {should} from 'chai';

should(); // this will patch `Object` so you can chain things like foo.bar.should

// tests

or

import 'chai/register-should.js'; // will patch `Object` _and_ make `globalThis.should` available
tiec7 commented 8 months ago

thank you @43081j , adding should() right after the import makes this works, however is terrible to see.

I used to use should just chained and imported with

var should = require('chai').should()

43081j commented 8 months ago

ah i see now

yeah unfortunately just because of how standard import syntax works in JS, we can't really chain it that way anymore. so it means you'd just have to do something like this:

import {should as loadShould} from 'chai';

const should = loadShould();
adrian-afl commented 8 months ago

Another option is to put it into mocha global setup so its only there and you don't need to worry about it in any other test file

tiec7 commented 8 months ago

ah i see now

Thank you, I was wondering how to explain myself better but you got it anyway!

Another option is to put it into mocha global setup so its only there and you don't need to worry about it in any other test file

Oook, may you just show me a small example or a link on how to do that? No problem if you can't, I hope i will figure out in the future when I'll learn more about testing

koresar commented 8 months ago

@jonkoops and @43081j My question was not answered yet. Please, do not close the issue as of yet.

Any plans to do at least security fixes to Chai v4?

43081j commented 8 months ago

we still haven't had chance to discuss it as a group yet, i'll try get hold of the others soon so we can answer you

my personal preference (not speaking for chai) would be to do security fixes for some time, until more people move (which could be a long time)

keithamus commented 8 months ago

This is an open source project and so it is up to the contributors we have to resolve issues and add features. The core maintainers (which is a group that changes over time) may raise PRs and make changes, but they can also mentor and guide contributors to do the same. This is to say if you are aware of a security fix that needs to be back-ported to 4.x.x you can raise a PR for it, against the 4.x.x branch, and a core maintainer will steer it in the right direction.

All the while the 4.x.x branch is open I'm happy to approve/merge PRs and cut releases for Chai 4. I can't give a definitive answer on when we'll delete that branch because ultimately it'll be "when it feels right". Certainly the majority of feature development will be done in Chai@5.

We typically get about 9MM downloads a week. Last week was particularly low because of the holidays but we saw about 4MM downloads. Of that 4MM, 100k of that is chai@5 (2.5%), about 300k for chai@3 (7.5%), and 3.6MM for chai@4 (87.5%). We do not get issues for Chai@3 and I don't think we'd cut a new release for the 3.x series, so one heuristic you could look at is this. Should Chai@4 reach 7.5% of our version distribution, I could foresee it being EOL then, but that's not the case today.

koresar commented 8 months ago

Amazing answer @keithamus This great communication skill is the reason why I am one of your followers on Twitter.

My questions are all sorted. I am happy to see much professionalism and community focused project governance.

Cheers people!

Have a great year. :) 🎄

KernelDeimos commented 7 months ago

In case anyone else comes to this thread trying to use mocha and chai together, using cjs modules;

For me, await import('chai') broke tests due to its asynchronous nature - none of the tests ran. This was regardless of whether I used async/await on my describe() and it() calls. I had to make use of mocha's before() for this:

let expect;
before(async () => {
    expect = (await import('chai')).expect;
});

I hope from this issue it's apparent how this change may have potentially cost many hours of human effort on part of all the developers who use this module in their unit tests. The issues surrounding ESM and CJS are known, plentiful, and widespread; IMO all of the very popular node.js modules should be responsible for ensuring things go smoothly with both systems, regardless of the opinions of the maintainers on ESM vs CJS.

jonkoops commented 7 months ago

IMO all of the very popular node.js modules should be responsible for ensuring things go smoothly with both systems, regardless of the opinions of the maintainers on ESM vs CJS.

That is very unreasonable, these libraries are maintained by folks mostly in their spare time, and asking them to maintain a complicated and error prone dual-module package is not trivial. Like @keithamus mentioned, they are still willing to maintain the 4.x release (which supports both ESM and CJS), so you don't need to move to 5.x if you can't right now. I'd consider that a more than solid 'smooth' migration strategy.

Once you convert your test suite to ESM (and you will have to do this eventually anyways), all of this becomes trivial to implement, as Node.js interop with CommonJS is pretty good. The other way around however, is terrible, and that is a design choice by the runtime, e.g. other runtimes such as Bun and Deno do no not suffer the same issue.

melroyvandenberg commented 5 months ago

I'm getting the following error during runtime:

Exception during run: file:///__w/../test/integration.test.js:2
import { use, should, expect, request } from 'chai'
                              ^^^^^^^
SyntaxError: The requested module 'chai' does not provide an export named 'request'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:134)
    at async ModuleJob.run (node:internal/modules/esm/module_job:217:5)

With the ESM JS code:

import { use, should, expect, request } from 'chai';
import chaiHttp from 'chai-http';
import app from '../src/index.js';

use(chaiHttp);
should();

// Down below, I use it like so within an describe & test mocha framework:
//  request(app).get(....)...

Despite I use: use(chaiHttp) that means request should be present.

43081j commented 5 months ago

not from the chai module

import {use, should, expect} from 'chai';
import chaiHttp from 'chai-http';

const chai = use(chaiHttp);

should();

chai.request; // now exists
MeastroZI commented 2 months ago

still getting error @43081j

" ReferenceError: request is not defined at Context. (file:///home/vinit/inter/Node_Proj/test/controllerTest.js:13:19) at process.processImmediate (node:internal/timers:476:21)"

import {use, should, expect} from 'chai';
import chaiHttp from 'chai-http';

const chai = use(chaiHttp);

should();

chai.request; 

describe('User Controller', () => {
  describe('GET /getUser/:userId', () => {
    it('should return a array of the object', async () => {
      const res = await request(app).get(`/listUsers`);

      expect(res).to.have.status(201);
      expect(res.body).to.be.an('array');

    });

    it('should return 404 error when user is not found', async () => {
      const userId = 'invalidUserId'; // Replace with an invalid user ID
      const res = await request(app).get(`/getUser/${userId}`);

      expect(res).to.have.status(404);
      expect(res.body).to.have.property('error', 'User not found');
    });
  });
});
43081j commented 2 months ago
chai.request;

// should be
const request = chai.request;

// elsewhere...
request(app).get(...);

// or just reference it directly
chai.request(app).get(...);