Open dreyks opened 1 year ago
Do you think we can do something about it? :) Like add an optional
key
toinjectEndpoints
?
What would happen if the key is accidentally the same? What's the benefit of that over prefixing your endpoint names, for example? (useFetchPostDataQuery
, useFetchUserDataQuery
, etc.)
additionally, each endpoint is exposed as api.endpoints.endpointName
in every instance, regardless of whether typescript knows about it or not. how would the key affect that?
Frankly, I don't really understand the use case of either overrideExisting set to true or false. Why would it be ok to disregard the fact that the endpoint names clash?
Hot module replacement could be one example where you would want to allow overriding the existing definition of the same name. #3157
I see... yeah, I've dug into the code a bit and now I can see it a bit more clearly...
cool - if you have any ideas for how to make the docs any clearer, I'm sure a PR would be welcome :)
I'll check if i indeed was not getting a warning about the same endpoint names
You should get warnings as long as you use the development build - we strip the warning in production to save on size.
Generally, I'd recommend longer endpoint names and aliasing the hook on export.
As for overrideExisting
, two possible ideas come to mind:
true
in HMR doing things like overrideExisting: module.hot?.status() === "apply"
this works for me personally, and for my pet app, for example. However, I think I'll have to come up with some sort of a custom lint rule to make people in the big app aware of the fact that they should use "descriptive" endpoint names.
ideally, I'd love it if the names wouldn't clash at all, like if each injectEndpoints
would create its own "pocket" in the api which would make it "safe" by default, but I understand that it might not be feasible both in terms of technical implementation and ideologically :(
I too assumed injectEndpoints
created its own little pocket until I started getting conflicts. Had a quick look at why overrideExisting: false
wasn't throwing any errors and I think I found the reason (at least for me).
if (
typeof process !== 'undefined' &&
process.env.NODE_ENV === 'development'
) {
console.error(
`called \`injectEndpoints\` to override already-existing endpointName ${endpointName} without specifying \`overrideExisting: true\``
)
}
By default webpack replaces process.env.NODE_ENV
with the string 'development
at compile time but it doesn't actually define window.process
so there is nothing there at runtime.
I was curious how React handles it but it seems to just assume it will be set? So if you use React from npm and don't use webpack or define process.env
manually you will get an error? Seems strange but maybe I'm missing something.
This is a standard JS ecosystem idiom for "only do this in development mode builds", yes:
As for typeof process
, it's a tradeoff. If we don't write that, the process.env.NODE_ENV
access will be a hard crash for anyone who isn't using any bundler like this at all.
It gets even more frustrating considering that there is also __DEV__
in some other environments. At some point we have to just make a call.
While researching on how to deal with endpoint name duplication, i stumbled on this issue.
In my case, there is no process
object at runtime, but there are environment variables and process.env
object.
Looking at @josh-cloudscape comment, shouldn't code which is checking for the environment look something like this?
try {
if (process.env?.NODE_ENV === 'development') {
console.error(
`called \`injectEndpoints\` to override already-existing endpointName ${endpointName} without specifying \`overrideExisting: true\``,
);
}
} catch {
// haven't come up with this block yet
}
@markerikson in our case for a larger application, we have never wanted to override the endpoint. In this case, it would be preferrable to throw an error, as we wouldn't want teams accidentally changing each other's function signatures. Though this is rare, do you think it could justify having a way to throw an error on override to prevent unintentional use leaking to production for applications like this? Something akin to -
overrideExisting: 'throw'
I have the same problem "getSupplierData" and "getSupplier" names are conflicting.
@markerikson in our case for a larger application, we have never wanted to override the endpoint. In this case, it would be preferrable to throw an error, as we wouldn't want teams accidentally changing each other's function signatures. Though this is rare, do you think it could justify having a way to throw an error on override to prevent unintentional use leaking to production for applications like this? Something akin to -
overrideExisting: 'throw'
@ffluk3 if you wanted to get a PR together with that option, I doubt we'd be against it :)
I'm converting a fairly big app to rtk-q. the app is heavily split into "features" and each feature is lazy-loaded, so we're using
injectEndpoints
. Different features are developed by different teams so it was just a matter of time before we started getting endpoint clashes with different features having something likeuseFetchDataQuery
oruseUpdateDataMutation
Do you think we can do something about it? :) Like add an optional
key
toinjectEndpoints
?~Additionally, I don't fully understand the
overrideExisting
option. With it being false by default it was rather surprising and tricky to track down why a wrong network call was suddenly made in a different part of the app.~ ok according to the doc it should've warned me in dev env, but for some reason it didn'tFrankly, I don't really understand the use case of either
overrideExisting
set to true or false. Why would it be ok to disregard the fact that the endpoint names clash?