diegohaz / arc

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

Import order situation/strategies #130

Closed zentuit closed 7 years ago

zentuit commented 7 years ago

I've run into a situation where I have a component at trying to import another component at its same "level" (ie Molecule). If the name of the component that I'm importing falls before the importer component in the sort order, then everything imports correctly. However, if the name comes after, then the component imported is undefined.

Ex: |- atoms |- molecules | |- AComponent | |- BComponent | |- CComponent

If BComponent has import AComponent from 'components' then everything is fine; AComponent is defined.

But if BComponent has import CComponent from 'components' then there is a problem; CComponent is undefined.

One way to solve that is to move BComponent up to Organism; but sometimes that could cause a chain reaction.

Another way is to require CComponent when needed if possible -- that's what I'm doing.

Are there any other strategies to deal with this situation?

diegohaz commented 7 years ago

Could you share the piece of code where you are trying to use the undefined component?

Most cases are solved this way:

import styled from 'styled-components'
import { CComponent } from 'components'

// fails
const StyledCComponent = styled(CComponent)``

// good
const StyledCComponent = styled(props => <CComponent {...props} />)``
zentuit commented 7 years ago

If I put console.log(CComponent) right after that import you have there, then it will report as undefined.

diegohaz commented 7 years ago

Yeah, that's normal. This is a circular dependency and Node should be able to resolve this at runtime, thus wrapping the component.

Can you try the code from my last comment?

zentuit commented 7 years ago

Ok I see the issue. I need to load the imported components into a lookup table so I can render the correct component based on the list of entries passed to my component.

I get a list of entries from the database, a mixture of articles, slides, etc. I use a meta data field to determine which component should be used.

Doing this:

import { Article, Testimonial } from 'components'

const lookup = {
  article: Article,
  testimony: Slide,
}

const Carousel = ({ items, ...props}) => {
    const slides = items.map((key, idx) => {
        const Comp = lookup[slides[key].meta.type]
        return <Comp key={idx} {...props} />
    }
<snip>
}

would result in Testimonials being undefined and so I would get a React error.

Moving the lookup creation into the Carousel component let everything resolve.

diegohaz commented 7 years ago

Nice. Thank you for the feedback. :)

keego commented 7 years ago

I love the dynamic importing done with components in arc and have been using it in my latest Vue.js project. I've hit this circular component dependency issue before and finally managed to find a solution that works. I first iterate over all the components and export an empty object. Then I reiterate over all the components and actually start requiring them, merging each module into module.exports. I got the idea from how webpack manages circular dependencies. Here's the code I'm using:

const merge = require('lodash/merge')

const req = require.context('.', true, /\.\/[^/]+\/[^/]+\.vue$/)

// This exports all components, resolving circular dependencies in two steps:
// 1. export empty objects in place of each component
// 2. require each component and merge it into the existing export for that module

const componentPaths = []

// export an empty object for each component
req.keys().forEach((key) => {
  const componentName = key.replace(/^.+\/([^/]+)\.vue/, '$1')
  componentPaths.push({ componentName, key })
  module.exports[componentName] = {}
})

// actually start exporting each component
componentPaths.forEach(({ componentName, key }) => {
  merge(module.exports[componentName], req(key).default)
})
diegohaz commented 7 years ago

@keego Wow!

Do you know if that works with styled-components when doing styled(CircularDependentComponent)?

Also, I wonder if is that _.merge really necessary. If so, it would be better to require('lodash/merge') instead of requiring the entire package (it increases the bundle size).

Anyway, it would be an amazing PR if you want to do that.

keego commented 7 years ago

@diegohaz Thanks!

Unfortunately, I just verified it does not work with styled(CircularDependentComponent). I'm not too surprised though as I've never seen circular dependencies get properly resolved without delayed evaluation. I think the issue there is still with the immediate invocation and, although it doesn't feel the most satisfying, I agree with your comment above about using styled(props => <CComponent {...props} />) instead.

Also you're very much right about including 'lodash/merge' instead of 'lodash', I just use lodash so much in my project that I've started requiring the whole thing. I just updated my comment :)

This mainly solves the specific issue described in the original post, and solves the circular dependency case for things that do allow for delayed evaluation (i.e. defining circular components in my vue project, and with certain cases of defining circular components in the arc project)

I can put this into a PR this week 👍

lastmaj commented 2 years ago

following the comment i went from export const LogoWrapper = styled(ReactSVG) to export const LogoWrapper = styled( props => <ReactSVG {...props} />)

I get the following error : Unexpected token, expected ","

any help ?

ezgitetik commented 1 year ago

@lastmaj you should convert file extension to .jsx (or .tsx) and import React. otherwise it does not recognize react component.