fix: mark context.request.loadedUrl and id as required inside the request handler
#2531
Conversation
github-actions
bot
added
t-tooling
B4nan
force-pushed
the
required-request-loaded-url
branch
2 times, most recently
from
0baee66 to
32aaf20
Compare
…uest/error handler The properties are still optional in other places like `preNavigationHooks`.
B4nan
force-pushed
the
required-request-loaded-url
branch
from
32aaf20 to
d30e84b
Compare
| // test that `request.loadedUrl` is no longer optional | ||
| expect(request.loadedUrl.length).toBe(sources[0].length); |
There was a problem hiding this comment.
- does the comment have to be there? it makes sense in the context of this PR, but later on, I think it won't be necessary
- why do you need to check the length instead of just comparing the string values?
There was a problem hiding this comment.
the test checks if loadedUrl is typed to string and not string | undefined which the latter would fail to compile here. you wont verify that if you compare the value, you need to call something on the value. the comment is there to explain this.
There was a problem hiding this comment.
Well, then the comment failed 😄 Why not check typeof then?
There was a problem hiding this comment.
That's the very same story, the test is about type level, not runtime values, those were correct already.
There was a problem hiding this comment.
Ah, I get it now. I guess there's no way to do expect, but on the type level. Could we reword the comment to something like "Check that request.loadedUrl cannot not undefined. If it could, accessing the length property would cause a Typescript error."?
There was a problem hiding this comment.
there are ways to test on type level too, often its about making assignments to explicitly typed variables (combined with using @ts-expect-error for cases that should fail). we could do the same here too. or we could use something like https://github.com/dsherret/conditional-type-checks
few examples of such tests here https://github.com/mikro-orm/mikro-orm/blob/master/tests/types.test.ts
There was a problem hiding this comment.
conditional-type-checks is pretty cool, if you think it's worth adopting it for cases like this, I'm in favor
The properties are still optional in other places like
preNavigationHooks.