Implement origin normalization in MockAgent for case-insensitivity and URL handling #4731
Conversation
lib/mock/mock-agent.js
Outdated
| dispatcher[kDispatches] = result.dispatcher[kDispatches] | ||
| return dispatcher | ||
| if (result && typeof keyMatcher !== 'string') { | ||
| const normalizedKeyMatcher = normalizeOrigin(keyMatcher, this[kIgnoreTrailingSlash]) |
There was a problem hiding this comment.
At this point, if stored correctly, the key matcher doe snot need new normalization, isn't it?
lib/mock/mock-agent.js
Outdated
| [kMockAgentGet] (origin) { | ||
| // First check if we can immediately find it | ||
| const result = this[kClients].get(origin) | ||
| const normalizedOrigin = normalizeOrigin(origin, this[kIgnoreTrailingSlash]) |
There was a problem hiding this comment.
On L59 the origin is already normalized by the caller, we can save another normalization, wdyt?
lib/mock/mock-agent.js
Outdated
|
|
||
| [kMockAgentSet] (origin, dispatcher) { | ||
| this[kClients].set(origin, { count: 0, dispatcher }) | ||
| const normalizedOrigin = normalizeOrigin(origin, this[kIgnoreTrailingSlash]) |
There was a problem hiding this comment.
Same as per the getter one, unless this is called explicitly somewhere else and the origin is not normalized, we can save this call as well
lib/mock/mock-utils.js
Outdated
| return origin | ||
| } | ||
|
|
||
| let originString = typeof origin === 'string' ? origin : origin.toString() |
There was a problem hiding this comment.
maybe?
| let originString = typeof origin === 'string' ? origin : origin.toString() | |
| let originString = typeof origin === 'string' ? origin : origin.origin |
There was a problem hiding this comment.
Yahh true, since we are just dealing with origin and its an instance of URL
So maybe I should just check if its a URL instance i can just return origin
lib/mock/mock-utils.js
Outdated
| let originString = typeof origin === 'string' ? origin : origin.toString() | ||
|
|
||
| try { | ||
| const url = new URL(originString) |
There was a problem hiding this comment.
If the passed arg is already an URL instance, maybe we can try to just infer the origin first?
lib/mock/mock-utils.js
Outdated
|
|
||
| try { | ||
| const url = new URL(originString) | ||
| url.hostname = url.hostname.toLowerCase() |
There was a problem hiding this comment.
The URL already does the parsing into lower case as part of the spec
…eter and simplify URL handling. Update MockAgent to utilize the new implementation for origin normalization.
…ethod, improving clarity and consistency in origin handling.
|
I removed the redundant normalisation in mock agent and kept the normalising logic to dispatch and get only. Also changed the logic of normalization |
… returning the origin of a URL instance, simplifying the logic and enhancing readability.
…ameter, allowing for flexible origin normalization. Update MockAgent methods to utilize the new parameter for improved origin handling.
|
I think passed option ignoreTrailingSpace is not affecting the result. In general, trailing space in the origin do not makes sense? |
|
Remove it! |
|
Okay I will remove it. |
|
Done |
|
Tests are not happy 😞 |
3 participants
This relates to #4703
Changes
normalizeOriginfunction inlib/mock/mock-utils.jsFeatures
Bug Fixes
MockAgent origin is case-sensetive #4703
Status