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 694 forks source link

mutating chai exports no longer works in v5 #1569

Open 43081j opened 8 months ago

43081j commented 8 months ago

As raised in chaijs/chai-http#310, the move to ESM actually had another breaking change we didn't catch

in the past, you could do something like this:

const chai = require('chai');

chai.use((ex) => {
  ex['oogabooga'] = 303;
});

chai.oogabooga; // 303

of course, in es modules, this will no longer work:

import * as chai from 'chai';

chai.oogabooga = 303; // TypeError, object is not extensible

chai.use((ex) => {
  ex['oogabooga'] = 303; // works but doesn't mutate `chai`
});

chai.oogabooga; // undefined

we can't really 'fix' this. imports are immutable objects when using *, so we can't just stick things onto the end.

it means we probably need to make a decision of what the 'new way' of doing this is. some suggestions:

cc @keithamus @koddsson

BryanHuntNV commented 8 months ago

Does this essentially mean that chai.use(chai-as-promised) is no longer supported?

43081j commented 7 months ago

until we fix it, it means you'd have to do this:

import * as chaiModule from 'chai';

const chai = chaiModule.use(chaiAsPromised);
BryanHuntNV commented 7 months ago

@43081j thanks for the clarification. Is there an ETA for a fix? I'm trying to prioritize upgrading to chai v5 vs waiting for the fix. We have hundreds of tests that need to be changed.

43081j commented 7 months ago

i haven't had chance to discuss it with keith and kristjan yet so its difficult to say

unfortunately it seems you will have to update your source either way, since there's no good way to provide a mutable export anymore.

it'll be a big PR for you, but if i were you, i would do the chai assignment i mentioned in my last comment for now at least. then if we settle on recommending another way, that will still work but you could move to the recommended usage another time.

basically a find and replace on the chai import in each file to be those two lines instead

BryanHuntNV commented 7 months ago

@43081j Thank you for the guidance. I appreciate that!

koddsson commented 7 months ago

I keep going in circles but this is starting to look like the solution to me.

  • require that consumers do something like chai = chaiOriginal.use(foo).use(bar);

I also wonder if we should guide plugins away from this mutating pattern. As I understand it it's not needed but more of a convenience?

43081j commented 7 months ago

it is pretty much convenience, because people used to import chai as a default import (const chai = require(...)).

this just doesn't work in esm anymore without exporting a chai const, which we shouldn't really do imo.

so my suggestion is to just recommend the chai = chai.use(...) pattern.

otherwise, i think we end up with something like chai.ext.myplugin

keithamus commented 7 months ago

FWIW this only impacts objects attached to the chai context from within a plugin. Regular assertions are attached to the Assertion prototype which is mutable in the module record. So only plugins which come with an additional "global" objects are effected. Those plugins can expose those objects as part of their module records, and we can also establish a convention that people should either import those objects from those libraries, or destructure the return value of the use() call. For example chai-http (an impacted plugin):

import * as chai from 'chai'
import {request}, chaiHttp from 'chai-http'
chai.use(chaiHttp)
request(...)

OR

import * as chai from 'chai'
import chaiHttp from 'chai-http'
const {request} = chai.use(chaiHttp)
request(...)

We can, as @43081j states, also add an object that is exported to simplify for other modules, so they don't need to repeat the use calls or additional imports between files:

import * as chai from 'chai';

chai.ext.request(...)

// OR

const {request} = chai.ext
request(...)
JakobJingleheimer commented 7 months ago

@43081j I'm one of the authors of node's modules (and ESM). I'm happy to help advise here 🙂

For instance, the package.json is misconfigured (you should be using exports). Fixing it is a breaking change.

43081j commented 7 months ago

what makes it misconfigured? i'm aware we could setup an export map but assumed node would figure it out without one when there is no dual-package stuff going on. is this not true?

we'd basically be exporting the exact files we have on disk, with no aliases, and no commonjs resolution. which i'd hope is the same as not having an export map

happy to add one if that isn't the case, it was more of an active choice (maybe a bad one) than a miss

rufreakde commented 7 months ago

Just wanted to add my observations as i was playing around with the 5.0.0 version.

So using this works

  let chai;
  let chaiRequest;

    await import("chai").then((result) => {
      chai = result;
      // Configure chai
      chai.should();
      chaiRequest = chai.use(chaiHttp);
    });

but there is a catch it works in my mocha run project only once! In the debugger I was able to observe. As soon as chai.should(); is called somehow the plugin disappears. This doesnt work:

  let chai;
  let chaiRequest;

    await import("chai").then((result) => {
      chai = result;
      // Configure chai
      chaiRequest = chai.use(chaiHttp);
      chai.should();
    });

So in the end I tried to use the upper solution as it works and I wanted to use the should() syntax in my tests. But as soon as another test of my suite starts that one will have a broken chai imported that cant "respond" a propert chaiRequest from its chai.use() function.

This means there is some scope issue here since I reimport now in every before inside its own describe to avoid scoping issues...

describe("Test 10", () => {
  ...
  let chai;
  let chaiRequest;

  before(async () => {
    await import("chai").then((result) => {
      chai = result;
      // Configure chai
      chai.should();
      chaiRequest = chai.use(chaiHttp);
    });
...
  });
...

Just my observations.

JoernBerkefeld commented 7 months ago

adding my issue on top of the list:

my test files used to work with chai 4 and the following:

import chai, { assert, expect } from 'chai';
import chaiFiles from 'chai-files';
chai.use(chaiFiles);

now, it breaks on the "import chai" part. The docs under https://www.chaijs.com/plugins/chai-files/ are unfortunately not yet updated. Bit of a mess to introduce a breaking change and forget to update the docs IMHO. I love the work that the authors of this have put in but in this very case they messed up :(

koddsson commented 7 months ago

The docs under https://www.chaijs.com/plugins/chai-files/ are unfortunately not yet updated. Bit of a mess to introduce a breaking change and forget to update the docs IMHO. I love the work that the authors of this have put in but in this very case they messed up :(

We're all the author/maintainer of this software. You can make a PR to update the docs ❤️

JoernBerkefeld commented 7 months ago

Well, if i knew how to then i'd do that instead of sharing my issue here, wouldn't I? ;-) For this project I'm just a user who is closing dependabot's update PRs for now without merging because there is no documentation on how to make things work with the current version. I'll be more than happy to keep docs updated for the OSS that I wrote if & when you find issues with its docs :)

koddsson commented 7 months ago

Well, if i knew how to then i'd do that instead of sharing my issue here, wouldn't I? ;-)

No worries! All the information that you need should be here: https://github.com/chaijs/chaijs.github.io. We can all work together to make chai better :)

JoernBerkefeld commented 7 months ago

you misunderstood, @koddsson: If i knew the solution I'd happily either create a PR or simply share it here. My issue is that I don't know the answer and don't have the capacity to analyze chai's code to understand this.

I was hoping that the team or person that worked on the recent major release should be well equipped to understand that they introduced a breaking change and explain the new solution for it... or roll back the change.

keithamus commented 7 months ago

I believe I've outlined the scope of the problem and solutions in my above comment: https://github.com/chaijs/chai/issues/1569#issuecomment-1882892715.

I think the two code examples I shared are perfectly reasonable solutions, they just need to be documented. I welcome anyone to raise a PR to the documentation so others can benefit from it.

What I'd expect to see from a good PR:

I think a guide on how users can use Chai with ESM and plugins would be a good page to add. I'd expect this to be linked in the Guide Index: https://github.com/chaijs/chaijs.github.io/blob/master/_guides/index.md underneath the heading "Basics", and could be titled something like "Importing Chai, and using plugins".

The new page should outline the basics on how to use Chai, that is - beyond the point of installation, how chai should be imported.

It should also include information on how to use a plugin. We can use chai-http as an example as it's A) a first party plugin and B) exposes a global.

The guide should feature information on how to install the plugin (chai.use), as well as how to correctly get the global from the plugin (in the case of chai-http either import {request}... or const {request} = chai.use. It should explain the pros and cons of each; largely the pro of import {request} is that it can be used between files without having to call chai.use().

What I'd expect to see from an excellent PR:

All of the above, plus an additional guide within the Making Plugins section, detailing how plugin authors can create plugins that expose globals in a sustainable way - this should steer authors toward exposing any global returned in the chai.use() in the module record as well, so...

// An example of a good plugin:

export const myGlobal = {...};

export default myPlugin() {
  chai.myGlobal = myGlobal; 
}
// An example of a plugin which may have issues:

const myGlobal = {...};

export default myPlugin() {
  chai.myGlobal = myGlobal; 
}
// An example of a plugin which may have issues:

export default myPlugin() {
  chai.myGlobal = {...}
}
ramicohen303 commented 6 months ago

I also experienced the "works only once" plugin issue that @rufreakde mentioned. I have multiple test files, each loading its own copy of chai and chai-http plugin. The first file works fine, all others fail. If I comment out the first file, the second file works fine, all others fail.

Looking at the code, I believe the issue cause is in chai.js, in the "use" function. The "used" array is global, and once a plugin is added to it, it cannot be added again. However, the exports object is instantiated on every call, but only the first instance gets the plugin.

For now, I created a factory function that returns a singleton chai and chai-http, and I use it in every test file. However, I believe this should be fixed properly.

43081j commented 6 months ago

you are indeed correct! it is a bug i think

we should call the plugin every time use is called, most likely. then delegate checking you haven't already been called to the plugin itself.

@ramicohen303 can you open a separate issue for that? along the lines of "use() function only returns plugin first time"

then we can discuss it separately there, as it is probably easier to fix than the overall change in how plugins work

ramicohen303 commented 6 months ago

@43081j done, see #1603

Thank you!

JoernBerkefeld commented 6 months ago

@keithamus I have indeed been able to use your guidance to fix my issue with it! Thank you for that! It took less effort than I thought it would in the end.

melroyvandenberg commented 4 months ago

I confirm, this drives me crazy. I downgrade back to v4.

e22m4u commented 2 months ago

The dirty workaround

import * as chaiTools from 'chai';
import chaiSpies from 'chai-spies';
import chaiSubset from 'chai-subset';
import chaiAsPromised from 'chai-as-promised';

// do recreate imported object
// to allow mutation
const chai = {...chaiTools};

// do invoke plugins directly
// to mutate the "chai" object
chaiSpies(chai, chai.util);
chaiSubset(chai, chai.util);
chaiAsPromised(chai, chai.util);

// do use the patched object
export {chai};
43081j commented 2 months ago

as mentioned elsewhere, you should be doing this:

import * as chaiModule from 'chai';
import chaiSpies from 'chai-spies';
import chaiSubset from 'chai-subset';
import chaiAsPromised from 'chai-as-promised';

const chai = chai
  .use(chaiSpies)
  .use(chaiSubset)
  .use(chaiAsPromised);

this is the correct usage if you need to access some properties your plugins add to the root chai object.

if you just want to hook in the plugin methods, you don't need a const chai and can just continue calling chai.use like before

the related bug has a fix in #1617