sinonjs / sinon

Test spies, stubs and mocks for JavaScript.
https://sinonjs.org/
Other
9.6k stars 769 forks source link

SinonFakeServer.respondWith not working with query params on 17.0.2 #2596

Closed 1bberto closed 1 month ago

1bberto commented 2 months ago

Describe the bug the respondWith seems to not work when the URL has a ?

To Reproduce Steps to reproduce the behavior:

I have this

const url =  `/api/FakeApi?hash=10101050&fakeData=true&isGet=true`;

const method = 'GET';

const response = [
  status,
  { 'Content-type': 'application/json', ...headers },
  JSON.stringify(body),
];

server.respondWith(method, url, response);

the code is breaking when server.respondWith is being called

the error is

TypeError: Unexpected MODIFIER at 12, expected END
at mustConsume (webpack://my-company/./node_modules/sinon/pkg/sinon-esm.js?:19288:15)
at parse (webpack://sensei/./node_modules/sinon/pkg/sinon-esm.js?:19346:9)
at stringToRegexp (webpack://my-company/./node_modules/sinon/pkg/sinon-esm.js?:19503:27)
at pathToRegexp (webpack://my-company/./node_modules/sinon/pkg/sinon-esm.js?:19581:12)
at Object.respondWith (webpack://my-company/./node_modules/sinon/pkg/sinon-esm.js?:17830:57)

Context (please complete the following information):

itchyny commented 2 months ago

We've been blocked by this issue, too. I noticed that the issue is caused by https://github.com/sinonjs/nise/pull/216. Rewriting all the usages of respondWith is a pain for us, but reading https://github.com/sinonjs/nise/pull/216, this seems to be an intended breaking change.

fatso83 commented 2 months ago

Thanks for providing this. This bug is totally on me. I am not sure how I missed James' very detailed explanations in his PR, which spells out how this is a breaking change and actually asks for input (which I also missed, must have been late).

My suggestion:

Thoughts, @43081j or @mroderick ?

fatso83 commented 1 month ago

An interim "fix" is that I rolled the latest tag to point to 17.0.1. Wiping node_modules should be sufficient.

43081j commented 1 month ago

i think that makes sense. let me know if you want any help sorting the new flag out etc

doesn't sound too difficult. basically default it to legacy behaviour which auto-escapes ? under the hood

fatso83 commented 1 month ago

The code repro is unfortunately not runnable (suggest using RunKit next time), so I have no 100% assurance this is fixed, but it should be fixed in the changes James shipped to Nise (enabling the legacy routes flag by default) should mean this is gone. Please test Sinon 18 and report back if this is not fixed.