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

Requires cycle warning #36

Open
shirshak55 opened this issue Nov 23, 2018 · 10 comments
Open

Requires cycle warning #36

shirshak55 opened this issue Nov 23, 2018 · 10 comments

Comments

@shirshak55
Copy link

shirshak55 commented Nov 23, 2018

It uses jsan packages which shows warning like this.

require.js:122 Require cycle: node_modules/jsan/lib/index.js -> 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.

is there anyway to get rid of such warning

@zalmoxisus
Copy link
Owner

Related to kolodny/jsan#25. Looks like that limitation was added somewhere in React Native, it doesn't have any issues, but better to handle on jsan part.

@zalmoxisus
Copy link
Owner

This is from React Native, right? If I'd have a repro would look into it, but if someone can do a PR on jsan part would be much appreciated. Or waiting till Metro decides to make it opt in as per facebook/metro#287.

@shirshak55
Copy link
Author

shirshak55 commented Dec 2, 2018

@zalmoxisus when i open try to debug react native using remote devtool it shows unknown call relay:check .and it shows following thing on devtools.

In screenshot you can see the message too.

screen shot 2018-12-02 at 12 55 06 pm

I have added you in repo so you can investigate.

Currently i am working it as private which i plan to release to open source later.
Thanks again.

@zalmoxisus
Copy link
Owner

Thanks for adding me. Unfortunately I'm not using RN atm and don't have an emulator on this machine to check now. Could you try to replace ./node-modules/jasn/lib/utils.js and restore-special.js inside your project form here. Most likely not.

I see it's warning not only when requiring the lib so should be quite annoying, but it would need to restructure the whole lib into one file which isn't quite sustainable just for a linting warning as discussed in kolodny/jsan#25.

@shirshak55
Copy link
Author

shirshak55 commented Dec 2, 2018

@zalmoxisus i replaced it but it seems everything is same nothing changed. I needed to create new file for restore-special.js file.

let me know if i can do anything.

@zalmoxisus
Copy link
Owner

Yes, I tried to move it to new restore-special.js file, thought it would ignore the cycle in run time this way, but seems no.

@shirshak55
Copy link
Author

@zalmoxisus is it due to package issue or which one? like many package etc are starting to showing up. If i don't use devtool it doesn't show any errors.

@zalmoxisus
Copy link
Owner

Yes, we have that in jsan, see kolodny/jsan#25 for more details. But that's not quite an issue with the package itself, it works as expected, but Metro from React Native is warning on that. As many suggested in facebook/metro#287, that should be be opt-in.

@shirshak55
Copy link
Author

hmm should we ignore it or ? . According to metro they are trying to protect us from code with cycle that can create problem in future.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Dec 2, 2018

I think yes, nobody had any problems in several years with the way the cycle is setup there. Just the warning flood is annoying.

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

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

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

2 participants