chaijs / chai-http

HTTP Response assertions for the Chai Assertion Library.
http://chaijs.com/plugins/chai-http
634 stars 113 forks source link

move to ESM (Chai 5) #310

Closed Trickfilm400 closed 5 months ago

Trickfilm400 commented 1 year ago

Changes

Current Issues

keithamus commented 1 year ago

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

koddsson commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Sounds good to me 👍

43081j commented 6 months ago

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

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 commented 6 months ago

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.

Trickfilm400 commented 6 months ago

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 commented 5 months ago

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?