rtk-query: endpoint name clashes in a big app

[dreyks]

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 like useFetchDataQuery or useUpdateDataMutation

Do you think we can do something about it? :) Like add an optional key to injectEndpoints?

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't

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?

[EskiMojo14]

Do you think we can do something about it? :) Like add an optional key to injectEndpoints?

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

[dreyks]

I see... yeah, I've dug into the code a bit and now I can see it a bit more clearly...

[EskiMojo14]

cool - if you have any ideas for how to make the docs any clearer, I'm sure a PR would be welcome :)

[dreyks]

I'll check if i indeed was not getting a warning about the same endpoint names

[phryneas]

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:

• setting it to true in HMR doing things like overrideExisting: module.hot?.status() === "apply"
• I used it once to have a normal api in place, and override it with an "offline" functionality on demand, which would read data from a local database instead.

[dreyks]

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 :(

[josh-cloudscape]

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\``
  )
}

https://github.com/reduxjs/redux-toolkit/blob/e92993813a95d9165e0df77cf52573d6021a6fa8/packages/toolkit/src/query/createApi.ts#LL335C11-L342C12

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.

[markerikson]

This is a standard JS ecosystem idiom for "only do this in development mode builds", yes:

• https://overreacted.io/how-does-the-development-mode-work/

[phryneas]

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.

[daantipov]

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
}

[ffluk3]

@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'

[emreakdas]

I have the same problem "getSupplierData" and "getSupplier" names are conflicting.

[EskiMojo14]

@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 :)

[ffluk3]

@EskiMojo14 I've thrown up a solution here and am happy to help take it through the review cycle!