Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@xstate/inspect should not bail in production #2020

Closed
mattpocock opened this issue Mar 20, 2021 · 5 comments · Fixed by #2046
Closed

@xstate/inspect should not bail in production #2020

mattpocock opened this issue Mar 20, 2021 · 5 comments · Fixed by #2046

Comments

@mattpocock
Copy link
Contributor

mattpocock commented Mar 20, 2021

@xstate/inspect works when process.env.NODE_ENV === 'development', but fails when NODE_ENV === 'production'. This is due to this line of code:

https://github.com/davidkpiano/xstate/blob/f3870f837e7d86bd4e3a1d28485e76b026579291/packages/core/src/devTools.ts#L43

Reproduction:

https://github.com/mattpocock/xstate-inspect-repro

Instead of bailing here, we should leave it up to the user whether or not they choose to have devTools: true in production

@Andarist
Copy link
Member

Right, the integration with Redux DevTools is available in production (from what I see in the source code) so this should be too.

We probably should put a doc in place to explain to people how to configure stuff properly though so they don't start opening popups in production by accident 🤔

@SimeonC
Copy link
Contributor

SimeonC commented Mar 22, 2021

I too tracked it down to this. I raised an MR without thinking too deeply about it, this morning it occurs to me that we have 2 checks for this, as shown below. IS_PRODUCTION and global.__xstate__.

https://github.com/davidkpiano/xstate/blob/f3870f837e7d86bd4e3a1d28485e76b026579291/packages/core/src/devTools.ts#L43-L47
https://github.com/davidkpiano/xstate/blob/f3870f837e7d86bd4e3a1d28485e76b026579291/packages/core/src/devTools.ts#L33-L35

I think we can just remove the IS_PRODUCTION check here and leave the check to whether the dev tools global has been set or not.

We probably should put a doc in place to explain to people how to configure stuff properly though so they don't start opening popups in production by accident 🤔

I don't think we need anything extra to do this, the popup is triggered from @xstate/inspect and isn't actually checking for IS_PRODUCTION anyway so changing this shouldn't change whether the popup opens in production or not. All the dev tools code is doing in xstate core is dealing with that global __xstate__ devTools object.

https://github.com/davidkpiano/xstate/blob/f3870f837e7d86bd4e3a1d28485e76b026579291/packages/xstate-inspect/src/browser.ts#L131

@SimeonC
Copy link
Contributor

SimeonC commented Mar 27, 2021

@Andarist what do you think about the above just removing the is prod flag?

@Andarist
Copy link
Member

I don't think we need anything extra to do this, the popup is triggered from @xstate/inspect and isn't actually checking for IS_PRODUCTION anyway so changing this shouldn't change whether the popup opens in production or not. All the dev tools code is doing in xstate core is dealing with that global xstate devTools object.

You are right. I think it should be OK to remove this production check then - unless @davidkpiano doesn't think so.

@davidkpiano
Copy link
Member

I don't think we need anything extra to do this, the popup is triggered from @xstate/inspect and isn't actually checking for IS_PRODUCTION anyway so changing this shouldn't change whether the popup opens in production or not. All the dev tools code is doing in xstate core is dealing with that global xstate devTools object.

You are right. I think it should be OK to remove this production check then - unless @davidkpiano doesn't think so.

That's fine. Just have to make a note in the docs that it's important to prevent the inspector from running in prod, if that is not desirable.

SimeonC added a commit to SimeonC/xstate that referenced this issue Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants