Throw an error in case Provider not being used

[RSchneider94]

Hi community,

I'm using this library in one of our projects for a long time. Today we have found a bug in our application, because someone has moved the <ModalProvider /> down in the component tree and it wasn't noticed until now, because we have a specific scenario where we use it and it was above the <ModalProvider />. Then I've started wondering how there weren't some errors, like usually happens when you don't use the Provider, and tries to call useContext e.g.:

const useSomeContext = () => {
  const context = useContext(SomeContext);
  if (context === undefined) {
    throw new Error('useSomeContext must be used within a SomeContextProvider');
  }
  return context;
};

Then I took a look at the file node_modules/mui-modal-provider/dist/mui-modal-provider.cjs.development.js and saw that on line 305 the library is making use of the useContext there. Couldn't this solution I have proposed above be applied there, just to avoid "hidden bugs" like this one we have gone through? I'd be open to contribute if that's something interesting to be applied.

[Quernest]

Hi community,

I'm using this library in one of our projects for a long time. Today we have found a bug in our application, because someone has moved the <ModalProvider /> down in the component tree and it wasn't noticed until now, because we have a specific scenario where we use it and it was above the <ModalProvider />. Then I've started wondering how there weren't some errors, like usually happens when you don't use the Provider, and tries to call useContext e.g.:

const useSomeContext = () => {
  const context = useContext(SomeContext);
  if (context === undefined) {
    throw new Error('useSomeContext must be used within a SomeContextProvider');
  }
  return context;
};

Then I took a look at the file node_modules/mui-modal-provider/dist/mui-modal-provider.cjs.development.js and saw that on line 305 the library is making use of the useContext there. Couldn't this solution I have proposed above be applied there, just to avoid "hidden bugs" like this one we have gone through? I'd be open to contribute if that's something interesting to be applied.

Hi @RSchneider94
Thank you for your comment, I will update this in the near future :)

[Quernest]

This is available in version 2.3.1

[michaeltford]

This change broke our application. We have a component that allows for modals to be optionally added. Perhaps an options flag? My suggestion would be to have an option {strickProviderCheck: false} that can be overwritten to {strickProviderCheck: true} for @RSchneider94's use case.

[Quernest]

This change broke our application. We have a component that allows for modals to be optionally added. Perhaps an options flag? My suggestion would be to have an option {strickProviderCheck: false} that can be overwritten to {strickProviderCheck: true} for @RSchneider94's use case.

Hi @michaeltford, could you provide any examples of it?

[RSchneider94]

This change broke our application. We have a component that allows for modals to be optionally added. Perhaps an options flag? My suggestion would be to have an option {strickProviderCheck: false} that can be overwritten to {strickProviderCheck: true} for @RSchneider94's use case.

Hmm, as far as I know it's kinda like a "common pattern" to have such "enforcement" of the Providers to be present while using Contexts. Not sure if adding a flag would be the prettiest way of handling this. But I'll let you send the examples that you have there and what @Quernest decides about it and I'll keep following here for any updates.

[michaeltford]

@RSchneider94 I am not sure what you mean by an Example

My component allows for modals to be optional. If they are not wrapped then we 'disable modal dialogs'.

const modal = useModal();
if (!modal) {
 // disabled button
}

Now there is no way to support this.

I am also not sure that throwing an exception is the "common pattern". MUI useTheme() does not throw an exception if you haven't wrapped in a provider. Neither does React.useContext.

// returns undefined if no Provider
const context = useContext(SomeContext);

It is my experience that the consumer believes that undefined is an error then they should throw the exception. (This is consistent with React and the reason that useContext does not throw an exception.

My suggestion (if you wanted to throw an error) was to make this an opt in) like

// throws an exception if no provider is present.
const model = useContext({
   enforceProvider: true
});

[Quernest]

@RSchneider94 @michaeltford I can suggest using console.warn instead of throw new Error, what do you think?

[michaeltford]

@RSchneider94 This is a bit of a use case driven problem. For my use case not defining a modal is not an error. (I can appreciate that in some or even most use cases it is.)

Honestly a console.warn would be worse in that it is not 'capturable'.

The way that I think about Context is as a contextual hashMap (I think React documentation refers to it this way.) Given my view it should behave like a map.

  // if doesn't exist then returns undefined, does not throw an error or log to console.
  const value = hashMap.get(key);

Additionally, undefined as a return value is more informative and a client can decided how to interpret it (as this value means 'no provider provided'. If an Exception is throw then the client has no idea why (unless they do a string compare on the error message which is really messy).

It's not an insurmountable problem I can wrap my useModal in a try catch (I think).

[Quernest]

@RSchneider94 This is a bit of a use case driven problem. For my use case not defining a modal is not an error. (I can appreciate that in some or even most use cases is it.)

Honestly a console.warn would be worse in that it is not 'capturable'.

The way that I think about Context is as a contextual hashmap (I think React documentation refers to it this way.) Given my view it should behave like

  // if doesn't exist then returns undefined, does not throw an error or log to console.
  const value = hashmap.get(key);

It's not an insurmountable problem I can wrap my useModal in a try catch (I think) but undefined is actually more informative in that I can decided for myself if this is valid where throw an Exception doesn't allow me to programmatically respond (unless I do a string compare on the error message).

IMO, in implementation with throw new Error breaking code like yours, console.warn will notify the inattentive developer that he forgot to add the ModalProvider instead of looking for reasons why his modal isn't showing.

[michaeltford]

What if the developer is not inattentive but wants to allow the ModalProvider to be optional (as is my use case?) Now they will have a console.warn messge in their application?

[Quernest]

What if the developer is not inattentive but wants to allow the ModalProvider to be optional (as is my use case?) Now they will have a console.warn messge in their application?

Agree, that would be annoying.

[michaeltford]

I am sympathetic the the originator of this defect but I don't think a library should be trying to guess what client is doing. React.useContext returns undefined if the value is not in the Context Tree.

IMO you should: 1. Provide the most flexible API (an exception is not flexible unless you provide error reasons). 2: Be consistent with React.

Given the original use case if you are refactoring code and don't have providers and contexts in the correct order a bug is sort of expected. (again think of the react.useContext use cases).

[Quernest]

@michaeltford about the code ... how do you have such a case earlier, because even if there was no provider there would be an object with a fallback?

const modal = useModal();

if (!modal) {
 // disabled button
}

[michaeltford]

I just reviewed my code. You are correct. I actually wish I could do the !modal check. Here is my actual code. My code is problematic in that it assume any overrides don't match the fallback id.

const { showModal } = useModal();
if (showModal().id === 'id') { // detect if modal is not set
 // disabled
}

so... it seems the root issue is that there should not be a fallback. Then undefined can either be handled or would create a 'fast fail' scenario. (I now see how a fallback stub could mask issues).

[Quernest]

I just reviewed my code. You are correct. I actually wish I could do the !modal check. Here is my actual code. My code is problematic in that it assume any overrides don't match the fallback id.

const { showModal } = useModal();
if (showModal().id === 'id') { // detect if modal is not set
 // disabled
}

so... it seems the root issue is that there should not be a fallback. Then undefined can either be handled or would create a 'fast fail' scenario. (I now see how a fallback stub could mask issues).

On the other hand, the version without fallback imposes additional conditions when the useModal hook is invoked.

For example:

const { showModal } = useModal(); // bye destructuring

// use it with additional checks
const modal = useModal()

if (modal) {
  showModal()
}

[michaeltford]

Hmm. That's true. ...

Perhaps the best answer is to:

1. Have a fallback (destructing is nice)
2. export the fallback modal (so that my usecase can do an === compare and disable).
3. Move the exception to the fallback functions.

This would

1. Allow the existing API.
2. Fail fast for people who don't do an explicit check
3. Allow for no provider detection (my usecase)

The existing apis wouldn't change

const modal = useModal();
if (modal === FallbackModal) {
  // disable
} else { 
  const { showModal } = modal;
  // showmaol
}

[Quernest]

Hi, I made a publish of the new 2.4.0 version

@RSchneider94 please update package and add the following code somewhere globally (in App or index file). This includes the behaviors we talked about above.

import { setModalConfig } from 'mui-modal-provider'

setModalConfig({ enforceProvider: true })

@michaeltford nothing will change for you, just update to 2.4.0

[michaeltford]

thank you @RSchneider94 . I just pulled and my issue is resolved.

[RSchneider94]

Thanks guys! @Quernest @michaeltford .. I'll do it soon!