MadeInHaus / react-starter

A client-side SPA starter bundled with React, Redux, CSS Modules, React Router 4, and Webpack 4
MIT License
9 stars 2 forks source link

Convert all `.jsx` files to `.js`, remove magic imports #25

Closed stevescavo closed 5 years ago

stevescavo commented 6 years ago

Seems as though the majority of the javascript community has moved away from distinguishing between jsx and js files. Reference: https://github.com/airbnb/javascript/pull/985#issuecomment-239145468

TL;DR: The distinction is annoying, especially in light of transpilers like Babel.

chrisjcodes commented 6 years ago

Agreed πŸ‘πŸ»

chrisjcodes commented 6 years ago

@stevescavo This is not a simple as it might seem. If we switch to js only we lose the ability to separate out which files are in fact React components and which are just JS files like utils. Our components index file uses this to auto export all the components so we can do this:

import { ComponentOne, ComponentTwo } from 'components'

Also if we needed to run just components through something else in webpack, we would have to know which files are components

stevescavo commented 6 years ago

I'm not a huge fan of the auto export "feature" as it's not completely obvious that every component in that directory will be bundled – even if it's not actively being used. I'm also leaning towards including an index file with each component directory (alongside .scss, .spec, [component].js, etc) where we can export the component so we don't miss out on too much syntactic sugar, i.e. deconstructed imports.

chrisjcodes commented 6 years ago

I am a fan of the following component structure

Where index.js is the component file.

We should discuss with the team if the aliased export, I.E. import { Component } from 'components', is something they would like to keep or go back to explicit imports. One thing we lose is the ability to freely reorganize where a component lives in the folder structure. You would have to update all import paths when you move a component

chrisjcodes commented 6 years ago

@MadeInHaus/mike-stewart Do you want to open a branch against this issue and update all the file names to match the above structure. You will also have to update the storybook config and the components index glob to find the correct files

rynocouse commented 6 years ago

I prefer following component structure:

ComponentName/ index.js (imports + exports the container or presentation) ComponentName.container.js (Defines Container) ComponentName.js (Defines Presentation) story.js style.scss test.js

chrisjcodes commented 6 years ago

@rynocouse I disagree. Containers are abstractions and shouldnt be a one-to-one with a component. Containers should be separated when the same logic needs to be shared among unrelated components or the presentational component can be used with different data.

You could separate the presentational component and have index be a pure passthrough for it but seems like were creating an unneeded extra file

claus commented 6 years ago

I dislike naming everything the same (i.e., index.js, etc) because my editor tabs are all gonna be Β―\_(ツ)_/Β―

claus commented 6 years ago

I don't have any better ideas either though, so i shut up.

rynocouse commented 6 years ago

@unruffledBeaver I disagree that a container component can't be one-to-one with a presentation. Yes it's and extra file that breaks out all the business logic from presentation component. There is nothing stopping anyone from creating abstract Containers with this pattern. That said this naming convention still hold up:

ComponentName/ index.js (alias for ComponentName, allows for import) ComponentName.js (defines ComponentName) story.js style.scss test.js

Also it seems to be convention amongst languages that defined classes have a matching filename. IMO we're only using index.js for import sugar

chrisjcodes commented 6 years ago

Seems like an unneeded extra file to me, but I'm open to discuss it further / try it out if the group thinks that is a sound approach

rynocouse commented 6 years ago

Way I see it we have to pick a approach: Implicit or Explicit. or maybe it's possible to mix the two.

IMO here's the breakdown (let me know if I'm missing something): Pros:

Cons:

Caveats:

Maybe it's not a one or the other argument. It's possible that we can glob only highly reusable "UI Kit" components eg: Button, Input as a single ui import

import { Button, Input, Link } from 'components/ui'; 
import Carousel from 'components/Carousel'; 
chrisjcodes commented 6 years ago

import Carousel from 'components/Carousel'; Would this mean there is no subdirectories at all and all components just live in one folder? If so I'm not a fan of that.

import { Button, Input, Link } from 'components/ui'; I feel like this would be the same as what we have now as I'm not sure, other than containers what wouldn't be UI

rynocouse commented 6 years ago

UI is really is being used as "UI kit". It's for atoms that are actually reused throughout the app. This folder structure is a play on the Fractal approach. Example folder structure:

/App
    index.js
    App.js
    store.js (combines all reducers via glob **/reducer.js)
    constants.js
    /Header
        index.js
        Header.js
        styles.scss
        test.js
    /FilmsSlider
        index.js
        actions.js
        reducer.js
        FilmsSlider.js
        styles.scss
        test.js
        /FilmsSlide
            index.js
            FilmsSlide.js
            styles.scss
            test.js
    /UI ("namespace" folder so we don't crowd the root. These are highly reused "dumb" components, essentially our projects ui-kit folder)
        Button/
            index.js
            Button.js
            styles.scss
            test.js
        FilmPoster/
            index.js
            FilmPoster.js
            styles.scss
            test.js
        Link/
            index.js
            Link.js
            styles.scss
            test.js
        ... all other ...

In this example FilmSlide is implied to be a component of concern to FilmsSlider. We don't try and fool ourselves and say it's an atom because it's built in a reusable way. Practically speaking it's truly purpose built for the FilmsSlider. Because FilmSlide is nested inside FilmsSlider the API should be "scoped" to FilmsSlider. It can change it without worry of breaking other components.

Another way to think about it is implied atomic: App(ecosystem)/FilmsSlider(organism)/FilmsSlide(molecule)

Say we have to build a FilmPoster that's actually becomes reused in more than one place, we would promote it either to top level App/FilmPoster or into our UI folder App/UI/FilmPoster. Now we know that the changing the api will be consequential to may components.

chrisjcodes commented 6 years ago

So this would be a complete departure from atomic then? I think it comes down to the needs of the project and how code is written for it. We could arguably have made that slide component less coupled and reused it if there was a reason to. In your example it does make sense that you could use the one-to-one approach but only because of how we wrote it.

In my experience so far, the lower parts of the atomic approach (atoms, molecules) are the most reusable pieces that you build into less reusable pieces. Really it comes down to a couple things in my mind

  1. Does the pattern organize our code well?
  2. Does it stay out of the way of the actual work?
  3. How much does it add to the cost of refactoring?

IMO the atomic approach with the "magic" import was the simplest because the hierarchy is familiar and I would start by creating something as an atom and then promote it up the atomic model if it began to compose other components.

TLDR; There is no silver bullet here. I see advantages and disadvantages to both and the trade-offs of using one or the other depend on the project

stevescavo commented 6 years ago

What I've learned since we've gone atomic is that no one knows where to keep their components. Is this a "molecule"? What constitutes an "organism"? What does HAUS have against "cells"? Is this component even reusable? Why am I asking so many damn questions?... I'M JUST TRYING TO CREATE A DAMN CAROUSEL SLIDE COMPONENT! πŸ˜‚

I've heard this across the board with our team.

The way we're currently assigning components to this fuzzy ecosystem metaphor is just another level of abstraction, another thing to think about, and another decision to make. It's also hard to explain because everyone seems to have varying opinions on where something should be placed. Our designers aren't even thinking in these terms so there's no win there either.

Inversely, fractal is intuitive. File systems are often organized this way. It's parent/child folders for the most part.

I'm in agreement with @rynocouse here – there are a lot of components that simply aren't reusable. That doesn't mean they shouldn't be abstracted (pros: css encapsulation, unit testing, readability, etc), but we should organize them with their parent/child relationship intact. It's simple, intuitive and requires no explanation. Furthermore, we get the best of both worlds when we keep a generic set of "atomic" ui components (i.e. <button>, <input>, <figure>, etc) alongside these fractally-organized custom application components. The UI components can be built into our starter, and leveraged at the developer's discretion. And because they aren't being "magically" exported, there's no risk of unintentionally bloating one's js bundle(s).

I don't see this as a big deviation from the core concepts of atomic design. If anything, it's closer to it, and closer to the way we naturally think about both component-driven design and a sound, intuitive file system.

rynocouse commented 6 years ago

@unruffledBeaver If by atomic you mean atomicly named folders, then yes.

The component import globbing seems to be a result of atomic folders. My understanding is it was added so components can be freely moved around folders without refactor. I ask myself, why are we moving components so often that we need magic import globbing? Seems to be a symptom of atomic folders not being intuitive enough, in so that we tend misclassify what part of the atomic structure it belongs in.

Does the pattern organize our code well?

IMO it feels more intuitive and easier to reason about the project and top-level render structure. No need to explain atomic folders. I believe components should still be built in a portable reusable way.

Does it stay out of the way of the actual work?

🀷

How much does it add to the cost of refactoring?

I don't find refactoring too much of an issue with find & replace.

stevescavo commented 5 years ago

Circling back on this. In general, the consensus is that magic imports are πŸ‘Ž . The primary concern is that they can swallow critical js errors. I also strongly feel that explicit imports are a good thing. Tracking their removal in https://github.com/MadeInHaus/react-starter/issues/39.