search

diegohaz / arc

React starter kit based on Atomic Design
https://arc.js.org
2.91k stars 295 forks source link

Improved resolution of circularly dependent components (#130) #316

Closed keego closed 6 years ago

keego commented 6 years ago

What

Improved how circularly dependent components are resolved by adopting webpack's resolution methodology: export empty modules first (as placeholders), then reiterate and actually export each module.

Why

This allows modules to hold references to their dependencies before they actually get exported :)

Caveats

as mentioned in #130 this still does not solve the styled(CircularDependentComponent) problem, BUT it does allow circularly dependent components to properly reference each other and work properly as long as evaluation doesn't occur until after the exports are fully resolved (i.e not having immediately invoked functions requiring dependent components).

codecov-io commented 6 years ago

Codecov Report

Merging #316 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #316   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          42     42           
  Lines         285    285           
  Branches       55     55           
=====================================
  Hits          285    285

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3b7983a...6d8bdc6. Read the comment docs.

diegohaz commented 6 years ago

Well done! Thank you, @keego.

Geczy commented 6 years ago

This commit breaks a few things, thoughts?

image

diegohaz commented 6 years ago

@Geczy Just looking at the log, I have no idea.

Was that working with the old index?

Geczy commented 6 years ago

Yep works with the old index

diegohaz commented 6 years ago

Alright, I'm gonna revert this for now until we figure it out.

keego commented 6 years ago

@Geczy @diegohaz looks like merging functions into objects doesn't work, I'll take a stab at a fix

keego commented 6 years ago

Okay, I did some more in-depth research of how component imports were resolving in my Vue project.

My issue: So this solved an interesting issue where I had

Page -depends-on-> NavHeader -depends-on->routes.js-depends-on->Page

where routes.js was being resolved with a reference to Page but that reference was being evaluated (routes: [{ component: Page }]) to undefined. The object-merge strategy I was using allowed this to instead be resolved as an empty object {} which the actual component object was merged into later.

Actual functionality of the change: Turns out this change actually solves a niche issue that allows for object exports to be evaluated before being fully resolved as long as their actual properties aren't needed until after being fully resolved.

TLDR: I think theres a fancy solution to make this work with exported functions as well but I don't think the added complexity would be worth it for this project as I doubt this niche functionality is useful for most cases. If an issue does come up that could benefit from this, then I'll tackle improving this and submitting another PR.