-
-
Notifications
You must be signed in to change notification settings - Fork 689
Implement origin normalization in MockAgent for case-insensitivity and URL handling #4731
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe?
| let originString = typeof origin === 'string' ? origin : origin.toString() | |
| let originString = typeof origin === 'string' ? origin : origin.origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😞 |
|
CI is red |
…ing slagh behavior consistent
|
I suspected this would happend, now the only other way I can think of is And move the logic of ignoreTrailingSlash back to the get as it was. Locally tests looks happy |
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
CI is red |
|
I see there are some tests hanging in the |
Cleanup the IgnoreTrailingSlash parameter in normalizeOrigin unit tests from mockutils.js
|
CI should be fine now, I tried in my local fork, here. The one that are failing, are failing for all the PRs out there I think. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4731 +/- ##
=======================================
Coverage 92.87% 92.87%
=======================================
Files 109 109
Lines 33826 33843 +17
=======================================
+ Hits 31415 31432 +17
Misses 2411 2411 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This relates to #4703
Changes
normalizeOriginfunction inlib/mock/mock-utils.jsFeatures
Bug Fixes
MockAgent origin is case-sensetive #4703
Status