chore: refactor for clarity #99
Conversation
|
* All tests reporting
@mcollina Here are my performance indicators comparing the previous implementation (before) to my current changes (after). Big picture, the change seems relatively small. Interestingly, the test results before my changes have less variation between test runs. Not sure what causes that... might just be a coincidence related to the load on my machine 🤷♀️ |
index.js
Outdated
| if (err !== null) return next(err) | ||
| // Falsy values are invalid | ||
| if (!resolvedOriginOption) { | ||
| return next(new Error('Invalid CORS origin option')) |
There was a problem hiding this comment.
This is new behavior? In past code, we are setting req.corsOriginAllowed = false on falsy values.
There was a problem hiding this comment.
If is an intentional behavior, should be tested as well.
There was a problem hiding this comment.
You are correct that this is a functional change. Before, if the origin option was a falsy value, CORS would just be disabled. If the origin option was a function that returned a falsy value (other than false), the accept origin header would be *.
This was an attempt to make the behavior from the origin option predictable in the following ways:
- The same behavior if the origin option is a value OR a function
- If the option is an invalid value (defined as any value not defined in the documentation), an error is thrown.
Clearly there is some risk to a change like this. However I would argue that it will actual make the module more understandable because the behavior is consistent. I welcome any discussion on this point 😄
There was a problem hiding this comment.
If your goal is to "refactor for clarity" then you should not also be introducing new functionality. Doing so makes the PR way more difficult to evaluate.
There was a problem hiding this comment.
I agree with @jsumners. I would like to see this change, but it's better in another PR.
There was a problem hiding this comment.
I agree with your point.
The challenge is that the old behavior was a bit tricky (and possibly unintentional):
- if the
originoption is falsy, CORS is disabled ☑️ - if the
originoption is a function that returnsfalse, CORS is disabled ☑️ - if the
originoption is a function that returns a falsy value other thanfalse, the allowed origin is*. (Note this behavior was never documented or tested) 😱
☝️ Handling the last case will add a lot of noise, but should be possible... I'll see if I can separate it into 2 PRs...
There was a problem hiding this comment.
It may not be ideal to replicate weird behavior. But consider that you have reviewers who probably know less about the code than you do at this point and they need to read the original code to understand the new code in order to properly review it. Any functionality change will be a radical change in this respect, thus making it prohibitively more difficult to review. For this case, I would note the change I want to make and then submit a follow up PR after the refactor PR is accepted.
There was a problem hiding this comment.
@RafaelGSS @jsumners I removed this change. I'll put it into another PR.
test/cors.test.js
Outdated
| const fastify = Fastify() | ||
| const origin = (header, cb) => { | ||
| t.strictEqual(header, 'example.com') | ||
| cb(null, undefined) |
There was a problem hiding this comment.
@RafaelGSS this test covers falsy values. The test is in the context of an origin function that returns a falsy value, but covers that case. Is ok for you?
index.js
Outdated
| _configureOrigin(origin) | ||
| function preflightHandler (req, reply) { | ||
| // Do not handle preflight requests if the origin was not allowed | ||
| if (!req.corsOriginAllowed) { |
There was a problem hiding this comment.
This is a new behaviour. Also technically the 404 code is not fitting the spec as the ressource is here but the CORS is not allowed. I'd rather use 400. Returning a 404 would be handled in a custom function.
There was a problem hiding this comment.
Glad to see an observant eye. This was changed in my last PR #96 .
This is due to the fact that I moved the preflight logic into the OPTIONS * route handler. There was some certain motivation for that (discussed in #96 ) but the general points were to:
- allow overriding of
OPTIONSroutes (seems to be the motivation behind The plugin treats every OPTIONS request as a preflight request #88 ) - make fastify-cors fit better into the fastify plugin paradigm. The previous implementation seemed to be a fairly strict port of express-cors, where middleware can either handle a request or pass it long to the next middleware in the chain. That model doesn't fit with plugins, hooks, and a "static" routing table that fasitfy uses.
Back to the question. This line is to allow for the case when the origin option is false or is a function that returns false. According to the docs, this should disable CORS (and the preflight handler). With middleware, this can just be checked when the middleware executes. With a static route, the route will always exist. 404 is to "simulate" the route not being there.
From a client perspective, if CORS is disabled, shouldn't the OPTIONS routes added by fastify-cors be "not found"?
Let me know if I didn't understand your point. Maybe the name of the field is misleading... perhaps it should be corsEnabled similar???
There was a problem hiding this comment.
I renamed this to corsPreflightEnabled which better describes the flag.
|
I removed any possible functional changes. This included:
I'll move those into another PR |
|
@zekth you approve? |
Checklist
npm run testandnpm run benchmarkand the Code of conduct
Refactored implementation for readability.
These important changes were made:
originoption is a function. The docs simply stated it should returntrueorfalseto allow the origin, but the implementation actually use the return value to process to the origin (i.e. it could be a string). This aligns fastify-cors express-cors.originoption is a function and returns a falsy value other thanfalse, for exampleundefined, the hook generates an error. The previous implementation treated this as*origin which does not seem intuitive and is not the way that express-cors works.originoption is a function and throws an error, the error is caught and passed into the hook callback (generating an error).