-
Notifications
You must be signed in to change notification settings - Fork 16
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
[FEATURE] Add test runner middleware #293
Conversation
32ad23a
to
22b52df
Compare
Static resources are copied over from OpenUI5.
22b52df
to
7e13b8a
Compare
lib/middleware/testRunner.js
Outdated
function createMiddleware({resources}) { | ||
return async function(req, res, next) { | ||
try { | ||
const pathname = req.path; // TODO: Other middlewares use the parseurl module here but I don't get why |
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.
I think req.path
might contain a query. That's why parseurl is used.
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.
Doesn't this stem from the time when our middlewares have been used in different server environments? See e.g. #183 .
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.
@codeworrior thank you for digging that out!
The express documentation makes it pretty clear that req.path
does not contain the query: https://expressjs.com/en/api.html#req.path
Since we already use parseurl elsewhere, I guess it doesn't hurt to offer the same compatibility as the other middlewares. I'll change the code 👍
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.
Done
To be compatible with connect middleware APIs
Looks good, but the test coverage decreased a little bit. |
@svbender Yes, but in |
As agreed with @codeworrior, including OpenUI5 code should not require a mention in the subcomponents part of LICENSE.txt. OpenUI5 is provided by the same entity (SAP SE) and under the same license (Apache 2.0) |
Static resources are copied over from OpenUI5.
Should we add this to our LICENSE.txt? 🤔