search

antoine-coulon / skott

All-in-one devtool to automatically analyze, search and visualize project modules and dependencies from JavaScript, TypeScript (JSX/TSX) and Node.js (ES6, CommonJS)
MIT License
645 stars 25 forks source link

Feature request: Distinguish safely-hoistable circular imports from those that will fail at runtime #87

Open ssalbdivad opened 1 year ago

ssalbdivad commented 1 year ago

If you're building a recursive structure like a DOM or a type system (in this case the latter), at some point you'll have to choose to either maintain a massive source file containing all your nodes or to judiciously allow some circular imports. The situation is summarized in more detail in a comment here.

Unfortunately, I'm not aware of any tools that will automatically detect when circular imports will be problematic at runtime, and the manual debugging process is painful if I make a mistake.

I'm not sure how viable this would be to add as a feature to skott, but if possible, it would single-handedly be justification for adopting it in projects like mine!

antoine-coulon commented 1 year ago

Hello there @ssalbdivad, thanks for continuing the discussion there :)

That's actually a super interesting topic, to be honest I didn't even think of differentiating types of cycles. From the first principles the goal was to provide the raw information and to let developers figure out what to do with cycles, because like unused modules it's complex to get it 100% right (+ depending on the module system it might change) and the last thing I would want is to provide false statement e.g: "this cycle is safe" but is indeed but harmful at runtime.

But it's definitely a great challenge and that would make skott the first tool to provide that type of information about cycles AFAIK. I'll probably try to poc that in the very near future (currently I'm mainly working on the rework of the webapp #72) and I'll keep you updated.

In some sort of ideal world given the thing is actually decently achievable, can I just ask what would be your desired outcome towards cycles information? Just as an example, could be like tagging cycles with levels of criticity:

  1. safe
  2. might be unsafe, raise a warning
  3. unsafe, raise a warning

I would tend to avoid ignoring "safe" cycles (as suggested in the eslint-plugin-import issue by @wycats) because even if they are "safe" it could still be a conceptual smell.

What do you think?

ssalbdivad commented 1 year ago

@antoine-coulon Yes this sounds perfect to me! I think it makes sense to continue detecting all cycles for the reasons you mentioned- in most cases, there is probably a better way to structure the code.

Would love to take a look if you're able to come up with a POC for this and test it out on some failing scenarios I've run into in the past!

antoine-coulon commented 1 year ago

@ssalbdivad, oh that would be great actually, I'm keeping that in mind and letting that issue opened so that I can kindly ping you when it's ready to be tested ;) Thanks again David!