move to ESM (Chai 5)

[Trickfilm400]

Changes

• Project files now use import / export syntax

Current Issues

• currently not usable because chai.request stays undefined after chai.use and I don't know why (I appreciate any help :) )
  • I guess because require has a different behaviour than any import syntax
• tests are because of this not working and need to be fixed

[keithamus]

@koddsson could you take a look at this please if you have time.

[koddsson]

I pushed some changes that fix the issue but there are some other issues that pop up. Mostly because there are still requires in the codebase which now error.

[koddsson]

@keithamus; I'm probably doing something wrong but chai.request seems to be undefined even though the function that's passed into chai.use sets chai.request to the correct object.

[Trickfilm400]

chai.request seems to be undefined even though the function that's passed into chai.use sets chai.request to the correct object.

Can confirm this, so could this be a upstream issue of the new chai 5 release?

[koddsson]

chai.request seems to be undefined even though the function that's passed into chai.use sets chai.request to the correct object.

Can confirm this, so could this be a upstream issue of the new chai 5 release?

Maybe! I think writing upstream tests to demonstrate and prove to ourselves that there's a problem would be a great first step.

[Trickfilm400]

I found ome upstream tests of the chai package and also some plugin tests: https://github.com/chaijs/chai/blob/5.x.x/test/plugins.js

The file is 7 years old, but the tests seems valid to me, which could indicate that the error is in this plugin here.

Maybe we can try and check if the chai v5 is working with another plugin to identify if it is an error in this plugin here or in the upstream chai package (This needs to update another plugin and these are also a few years old, but they would need to be migrated anyways to v5 or implemented directly into the main package (which I would prefer, but I don't know the concept of this))

Another more simplistic idea would be to create a empty sample plugin like in the unit tests to validate the plugin functionality with a real external plugin

I could try to check this issue with the most up-to-date plugin (https://github.com/chaijs/chai-spies), but I need to find some time for this

[Trickfilm400]

but chai.request seems to be undefined even though the function that's passed into chai.use sets chai.request to the correct object.

After tinkering around with all of this, I found that the return value of chai.use has the chai.request property set correctly (see in line https://github.com/chaijs/chai/blob/082c7e2548140813452d7ee0cf6d42052bc43c5c/lib/chai.js#L50C3-L50C18):

let customChai = chai.use(http)
customChai.request(/**/)

If you use this returned variable, the tests are working,
but chai.request returns undefined, regardless of how you set the .request property inside the plugin

This looks like a upstream issue, but even with the given (and updated) unit test (https://github.com/chaijs/chai/blob/main/test/plugins.js), I wasn't able to figure out how to register a global chai property, because this test only tests an assertion and even with the same function used in the test (Object.defineProperty), it stayed undefined

I'm out of my knowledge here and I'm thinking about an upstream issue to resolve this - because now that v5 is released, it could be useful to have a working plugins API

[43081j]

it is an issue in chai

by moving to esm, we lost module.exports as a mutable object, i.e. the use function used to mutate module.exports to include your export.

now it only mutates a fabricated exports object, which use returns.

we should track this in a chai issue as it will need discussion. in es modules, the old pattern isn't doable in most cases and/or is unnatural. so it may be that we have to introduce a new way of accessing extensions in 5.x (unfortunately does mean we have shipped a breaking change we weren't aware of! good catch)

[perrin4869]

I know it is very bad manners to plug your own plugin as a solution to a problem, but some time ago I put together chai-superagent precisely to avoid the kind of mutation problems with chai-http and to simplify the API, please give it a try if you're interested 🙂

[Trickfilm400]

Hi,
I've updated the tests and some other parts for the version 5 release (Using one new plugin use syntax)
-> Tests are passing now

The build process is failing, but I don't know anything about this process, so I'm unable to fix this easily

[keithamus]

@Trickfilm400 can you please rebase against the main branch, I've updated CI to drop coveralls which looks like it's failing the build.

[Trickfilm400]

If I want to run the npm run build command I get the following build error:

(...)

> chai-http@4.4.0 build:js
> simplifyify lib/http.js --outfile dist/chai-http.js --bundle --minify --debug --standalone chaiHttp

Error bundling lib/http.js
'import' and 'export' may appear only with 'sourceType: module'

Process finished with exit code 2

I think the simplifyify cannot work with esm modules, but I never used module bundlers before

[43081j]

we should just drop simplifyify IMO

i wonder if we should just ship the ES modules as-is and leave bundling up to consumers

what do you think @keithamus?

[keithamus]

Sounds good to me 👍

[43081j]

@Trickfilm400 in that case i think you just need to (roughly) do the following:

• remove the build:js npm script
• remove the mentions of dist/chai-http.js from README and docs/ (or update them to tell users to use the normal index.js instead)

this does mean it won't work in a browser anymore 🤔 (assuming it does now)

if we do want it to work in a browser, we'd have to make an entrypoint that doesn't import node standard libraries. not too sure what we want to do about that yet

[Trickfilm400]

I've updated it and made a note for the browser topic to use the v4 for now. If there is a solution in the future, it could be added again - personally, I will not use browser tests, I only need node tests

It seems to me that everything is finished, so I will remove the draft state, but feel free to mention anything that needs to be done.

[43081j]

the formatting should really be consistent (i.e. missing semi here) but i wonder if we should just do a follow up prettier PR if we don't already have it

[Trickfilm400]

yeah, setting up prettier would help a lot

We can create these follow-up PRs and then create the v5 release if this is finished (I didn't want to create all of this in this PR, as it would be too much I think)

[43081j]

looks good to me, though left a couple of questions/comments

@keithamus could you review too? this'd be a breaking change ofc, so need to take extra care reviewing if we need to update any docs etc

i think ill follow this PR up with some modernisations too (prettier, update some devDeps, etc)

[Trickfilm400]

i think ill follow this PR up with some modernisations too (prettier, update some devDeps, etc)

the devDependencies are up-to-date, they got updated in the master branch recently

I hope I didn't break anything with the rebase there while resolving the merge conflicts

[Trickfilm400]

Hi,

@keithamus any news about this PR?

The default branch got a few updates which would already be covered by this PR, which is a bit redundant work, so a merge would improve the situation

Is it possible to restart the unit tests of the pipeline? The tests are failing because of some network timeout, but locally everything works just fine?

[keithamus]

Thank you for your hard work on this @Trickfilm400! LGTM 👍