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

mutating chai exports no longer works in v5 #1569

Open
43081j opened this issue Jan 2, 2024 · 22 comments
Open

mutating chai exports no longer works in v5 #1569

43081j opened this issue Jan 2, 2024 · 22 comments

Comments

@43081j
Copy link
Contributor

43081j commented Jan 2, 2024

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:

  • require that consumers do something like chai = chaiOriginal.use(foo).use(bar);
  • introduce some kind of chai.extensions, a mutable object we store these things in

cc @keithamus @koddsson

@BryanHuntNV
Copy link

BryanHuntNV commented Jan 3, 2024

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

@43081j
Copy link
Contributor Author

43081j commented Jan 4, 2024

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

import * as chaiModule from 'chai';

const chai = chaiModule.use(chaiAsPromised);

@BryanHuntNV
Copy link

@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
Copy link
Contributor Author

43081j commented Jan 4, 2024

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
Copy link

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

@koddsson
Copy link
Member

koddsson commented Jan 5, 2024

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
Copy link
Contributor Author

43081j commented Jan 5, 2024

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
Copy link
Member

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
Copy link

JakobJingleheimer commented Jan 10, 2024

@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
Copy link
Contributor Author

43081j commented Jan 10, 2024

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
Copy link

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
Copy link

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
Copy link
Member

koddsson commented Jan 30, 2024

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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

keithamus commented Feb 1, 2024

I believe I've outlined the scope of the problem and solutions in my above comment: #1569 (comment).

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
Copy link

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
Copy link
Contributor Author

43081j commented Feb 13, 2024

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
Copy link

@43081j done, see #1603

Thank you!

@JoernBerkefeld
Copy link

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants