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

Odd crash on Expo when minified and in production mode #4443

Closed
trajano opened this issue Nov 22, 2022 · 17 comments
Closed

Odd crash on Expo when minified and in production mode #4443

trajano opened this issue Nov 22, 2022 · 17 comments

Comments

@trajano
Copy link

trajano commented Nov 22, 2022

Prior Issues

What is the current behavior?

The app crashes immediately. Note this does not occur if the app is not minimized or if in non-production.

Steps to Reproduce

I am unable to make a MVCE to show this.

But this can be recreated in my app https://github.com/trajano/spring-cloud-demo (it's not a trivial app, but to make it crash I when I add the following to https://github.com/trajano/spring-cloud-demo/blob/rework/expo-app/authenticated-context/AuthenticatedProvider.tsx

import { configureStore } from "@reduxjs/toolkit";
export const store = configureStore({
  reducer: {},
});

What is the expected behavior?

Not crash.

Environment Details

"@reduxjs/toolkit": "^1.9.0",
"react-redux": "^8.0.5",
"redux-persist": "^6.0.0",
"expo": "^47.0.6",
"react": "18.1.0",
"react-dom": "18.1.0",
"react-native": "0.70.5",
@trajano
Copy link
Author

trajano commented Nov 22, 2022

For me to fix

import { legacy_createStore as createStore } from "redux";
export const store = createStore((state = 0, action) => state);

I had to remove in lib/redux.js this block


/*
 * This is a dummy function to check if the function name has been altered by minification.
 * If the function has been minified and NODE_ENV !== 'production', warn the user.
 */

function isCrushed() {}

if (process.env.NODE_ENV !== 'production' && typeof isCrushed.name === 'string' && isCrushed.name !== 'isCrushed') {
  warning('You are currently using minified code outside of NODE_ENV === "production". ' + 'This means that you are running a slower development build of Redux. ' + 'You can use loose-envify (https://github.com/zertosh/loose-envify) for browserify ' + 'or setting mode to production in webpack (https://webpack.js.org/concepts/mode/) ' + 'to ensure you have the correct code for your production build.');
}

As to why it would work without it, I am not sure, it does not look like it should have any side effects. However, this fix does not propagate to @reduxjs/toolkit I think it's doing something extra but I am not certain what.

@timdorr timdorr transferred this issue from reduxjs/redux Nov 23, 2022
@markerikson
Copy link
Contributor

I'm kind of confused. The check you're pointing to is in the redux core createStore function, but this only happens if you use RTK's configureStore, and works fine with just createStore by itself?

@markerikson
Copy link
Contributor

@timdorr @phryneas so tracing back the history of those lines, it dates back to this issue/PR in late 2015:

Worst case I think we'd be okay to remove that warning?

@timdorr
Copy link
Member

timdorr commented Nov 23, 2022

I know I'm the one that originally merged this, but I think it's probably better to remove it. As we move towards ESM being more and more popular, relying on a Node-specific global is going to become less likely to be present and increase these kinds of issues.

At least, that's my general sense of things. I have no data to back that up, so just take it as a hunch.

@markerikson
Copy link
Contributor

Eh, I'm less worried about the NODE_ENV thing than I am the "does this 'non-minified code' warning even still make sense in 2022?" question :)

@trajano
Copy link
Author

trajano commented Nov 23, 2022

I'm kind of confused. The check you're pointing to is in the redux core createStore function, but this only happens if you use RTK's configureStore, and works fine with just createStore by itself?

The check I am poiting at is in lib/redux.js or redux. The toolkit does import * from redux so I was trying to hunt down what could cause it. Like I said the behaviour is sort of weird at least let me see a JS crash trace or allow me to wrap in a try-catch and log the error, but so far and I haven't been able to pull it out on its own.

@markerikson
Copy link
Contributor

@trajano : yes, I know, that's why I'm confused why you're saying this happens with RTK's configureStore(), but not just the core createStore(). That code is in the module-level redux/src/index.js file, so it runs as soon you import anything from redux. Therefore, this error shouldn't depend on whether or not you're using RTK.

@trajano
Copy link
Author

trajano commented Nov 23, 2022

Well a similar error occurs on RTK (just crash) but not l don't know what exactly is causing it. I am just pointing out that the patch I did on redux.js seems to fix createStore in redux. Maybe there's similar code in RTK, but by the time I can "see" it, the code is minified to an unreadable state already.

@markerikson
Copy link
Contributor

Yeah, I understand that you're seeing this crash when you call configureStore().

But because this code is in the redux core, I'm asking:

does this also crash if you just use const store = createStore()?

because the "RTK" part shouldn't matter here.

@trajano
Copy link
Author

trajano commented Nov 23, 2022

does this also crash if you just use const store = createStore()?

You mean

import { legacy_createStore as createStore } from "redux";
export const store = createStore((state = 0, action) => state);

Yes it crashes unless I remove that isCrushed check. So if that check was removed at least the "legacy" version of Redux would still work.

@phryneas
Copy link
Member

If that check was removed, all versions would - this is really just the core implementation.

@trajano
Copy link
Author

trajano commented Nov 23, 2022

If that check was removed, all versions would - this is really just the core implementation.

you mean it would cause crashes on other places if that isCrushed check was removed @phyrneas?

@markerikson
Copy link
Contributor

markerikson commented Nov 23, 2022

No. We're saying that there is only one place that check exists, in the redux core library index.js file. So it doesn't matter if you're using "legacy Redux" (the redux package), or Redux Toolkit (which depends on the redux package) - removing that check from redux would fix this everywhere redux is used.

In other words: This is not a "Redux Toolkit" problem. This is a "Redux core" problem.

I'm going to go ahead and transfer this issue back to the core repo.

@markerikson markerikson transferred this issue from reduxjs/redux-toolkit Nov 23, 2022
@trajano
Copy link
Author

trajano commented Nov 23, 2022

Ah ok. Well the fix I did for for core that's for sure. So it may be a diffrerent one. However, let's see if we can clear off that check at least I'd have legacy redux as an option.

However just having

import '@reduxjs/toolkit';

with minified and process.env === PRODUCTION is enough to cause my app to crash with no useful traces that I can find.

@markerikson
Copy link
Contributor

Yes, that's exactly my point.

Redux Toolkit, the @reduxjs/toolkit package, imports methods from the redux core.

That isCrushed check is in the redux/src/index.js module file.

It runs as soon as you import anything from the redux package.

Therefore, yes, importing RTK triggers this crash because importing anything from redux runs that check.

It's not RTK that's the issue. It's the redux index file.

@trajano
Copy link
Author

trajano commented Nov 23, 2022

It's not RTK that's the issue. It's the redux index file.

Hopefully that's all it is. Eagerly awaiting a release.

trajano added a commit to trajano/redux that referenced this issue Nov 25, 2022
trajano added a commit to trajano/redux that referenced this issue Dec 15, 2022
timdorr pushed a commit that referenced this issue Dec 17, 2022
Co-authored-by: Tim Dorr <[email protected]>
Fixes #4443
@markerikson
Copy link
Contributor

Okay, this is finally out in https://github.com/reduxjs/redux/releases/tag/v4.2.1 . Sorry for the delay!

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