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

Require Cycle: lib/index.js -> lib/cycle.js -> lib/utils.js -> lib/index.js #25

Open
rsml opened this issue Nov 7, 2018 · 7 comments
Open

Comments

@rsml
Copy link

rsml commented Nov 7, 2018

This issue appeared after upgrading to react-native@0.57.4 and react@16.6.0-alpha.8af6728". Somewhere in react or react-native they recently added automatic tracking for require cycles.

So every time I run my application I now get a warning from react native, saying:

Require cycle: node_modules/jsan/lib/index -> node_modules/jsan/lib/cycle.js -> node_modules/jsan/lib/utils.js -> node_modules/jsan/lib/index.js

Require cycles are allowed, but can result in uninitialized values. Consider refactoring to remove the need for a cycle.

I'm using jsan@3.1.11

@adamsdenniskariuki
Copy link

adamsdenniskariuki commented Nov 12, 2018

Getting the same issue here using react native 0.57.4. Seems to be coming from metro: metro issue 287

@kolodny
Copy link
Owner

kolodny commented Nov 22, 2018

I'm open to a PR to fix this. There isn't any concern with the way the cycle is setup since module.exports is never being redefined so it safe to ignore. I'm not sure why the default config is to warn/lint for packages in node_modules

@zalmoxisus
Copy link
Collaborator

zalmoxisus commented Dec 1, 2018

I'm not using React Native and don't have an emulator to check it, but I guess it's because of this part I introduced in #12:

jsan/lib/utils.js

Lines 50 to 53 in bba3737

case 'm':
return new Map(jsan.parse(rest));
case 'l':
return new Set(jsan.parse(rest));

restore from there is calling parse, while parse is calling restore (through decycle). Nothing bad in that, just that they are in different files/modules. Bringing them together in one file should make Metro happy, but we are losing in code usability. So the question is how crytical/annoying that warning is. I see there're no plans for facebook/metro#287 so far.

@zalmoxisus
Copy link
Collaborator

The solution is to patch Metro as indicated in facebook/metro#287 (comment).

Let's keep the issue open for now, so others can find.

@Enalmada
Copy link

Enalmada commented Dec 3, 2018

I just updated a sample app dependencies and getting this error in code that seems out of my control.
Seems like the temporary workaround is to put this in your root js file:

import { YellowBox } from "react-native";
YellowBox.ignoreWarnings(["Require cycle:"]); 

@kolodny
Copy link
Owner

kolodny commented Dec 5, 2018

This would require jsan to have react-native as a peerDep, something I'm not keen on. You could wrap this module in a module that makes that call and returns the jsan object. Shouldn't be too hard

@overengineered
Copy link

Another solution is to change utils.js so that it requires './' inside restore function. Then the warning will not show up whenever jsan module is imported as it does now, but only when parsing object that had cycle references.

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

No branches or pull requests

6 participants