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

require.context #131

Open
diegohaz opened this issue Feb 24, 2017 · 20 comments
Open

require.context #131

diegohaz opened this issue Feb 24, 2017 · 20 comments

Comments

@diegohaz
Copy link
Owner

I'm creating this issue to unify the discussions about require.context.

While that's very handy to import components, it comes with some drawbacks like #130, #113 and #128 (comment)

Personally, I don't have problems with it, the advantages are bigger than the issues for me. But, as many are having problems, we should consider removing it by default or at least give proper instructions on how to remove it.

If you are using this webpack feature (whether your project is based on ARc or not), please share your thoughts.

@zentuit
Copy link

zentuit commented Feb 24, 2017

I love the feature of just doing import { XYZ } from 'components'. (I just got caught doing some stuff "prematurely" 😁 .)

I'm not in favor of removing it by default.

@santino
Copy link
Contributor

santino commented Feb 28, 2017

Initially I did love the opportunity to import modules from 'containers' or 'components' but now I came across an issue which I don't seem to find an easy solution to.

I have a LoginPage container that map some props to the LoginPage (page) component which in turn render (among other things) a <LoginForm /> which is basically another container decorated with redux-form.

If in my LoginPage component I import the LoginForm container like this import { LoginForm } from 'containers' it doesn't work, because basically by the time this is imported in the LoginPage container which decorate the LoginPage component which requires the LoginForm container this container has not been exported yet.
If I instead import it directly from the path the file is in, like this import LoginForm from '../../../containers/LoginForm.js' it works correctly.

Sorry fo the headache, I hope the above scenario somehow make sense.
I couldn't figure out a way to get around this reference issue with using the import from 'container'; unless I'm missing something here I believe this could create serious limitations.

@zentuit
Copy link

zentuit commented Feb 28, 2017

@santino : that sounds similar to what happened to me. Basically, you have to use the components you import in function so they are defined when the function is called. See #130. By moving the use of the imported component into a function, everything works.

@diegohaz
Copy link
Owner Author

diegohaz commented Mar 1, 2017

Node resolves those circular dependencies at runtime. Just remember to wrap the components. These are the rules:

  • If you are importing a container or a component from the same atomic design level, just remember to wrap it

    // components/organisms/SomeOrganism
    import { Organism } from 'components'
    
    // bad
    const StyledOrganism = styled(Organism)``
    
    // good
    const StyledOrganism = styled(props => <Organism {...props} />)``
    
    // good - rendering the component is equivalent to wrapping it
    const SomeOrganism = () => (
      <div>
        <Organism />
        ...
      </div>
    )
  • On containers, always wrap the components

    // containers/OrganismContainer.js
    import { Organism } from 'components'
    
    // bad
    export default connect(...)(Organism)
    
    // good
    const OrganismContainer = props => <Organism {...props} />
    export default connect(...)(OrganismContainer)

Following those rules should be enough. We should add it to the new Wiki. Please, tell me if there's another use case.

@santino
Copy link
Contributor

santino commented Mar 1, 2017

Thanks @zentuit and @diegohaz for your suggestions; eventually wrapping the component on the container solved the issue.

This feels like a work around but not a big compromise to live with so, going back to the original topic, I agree that the benefits are more than the disadvantages hence I vote against removing it by default.
There is certainly need for more documentation around this as I can see people spending time to get to the source of the issue.

I also take the opportunity of this comment to thank you @diegohaz for sharing such a good work, your boilerplate is exactly what I was looking for.

@advance512
Copy link

Not sure how nicely require.context plays with tree-shaking, an important feature of Webpack.

Also, I personally love WebStorm's automatic imports (Alt + Enter or Cmd + Enter automatically adds the correct, most exact, import at the top). This is simply not applicable with require.context.

I've had issues running this code in SSR using universal-webpack. (Yes, I've basically gone and made a brand new boilerplate based on the PR I submitted to arc a few weeks ago... with support for toggling SSR on and off, and much easier SSR support for various libraries.) Not sure if this is relevant here, as webpack-isometric-tools are used instead.

Finally, I've had issues with this when trying to run the server for SSR without bundling. I have a development server that uses babel-require and other similar tools and allows a much easier, faster debugging experience for me. But, since there's no webpack involved, require.context doesn't work.

@diegohaz
Copy link
Owner Author

diegohaz commented Mar 9, 2017

Maybe we can add a npm script to automatically add files to index.js to be ran concurrently with npm start/npm run dev in watch mode?

@advance512
Copy link

https://github.com/gajus/create-index

This might be useful?

@declanelcocks
Copy link

@advance512 Am I correct in assuming it's an automated way of doing this below?

https://github.com/diegohaz/arc/blob/0b9cacb2767a77ac4ea55f0a8c8c5b6c757ebac0/src/components/index.js

Could be useful in making generating the above file automated and easy, but maybe it would create some confusion for new users who just want to understand how to add/import/export new components without having to read the README.

@advance512
Copy link

@declanelcocks yes, of a sorts.

The main point is not confusing WebStorm's new feature: auto import of React components.
https://www.jetbrains.com/webstorm/whatsnew/

This is also relevant for actions, and in my case for GraphQL queries.

@declanelcocks
Copy link

declanelcocks commented Mar 21, 2017

@advance512 Out of curiosity did you try to implement something like the above?

@advance512
Copy link

No, I actually completely discarded this method. I also stopped exporting as default, rather I export by the identifier (e.g. control, class, whatever) name. It makes auto-imports almost as good as in Python.. and I don't care about the paths as I never manually write them.

@diegohaz
Copy link
Owner Author

diegohaz commented Mar 22, 2017

I was thinking about how would look a generic CLI to completely replace our usage of require.context. While I was writing I realized that it was becoming overkill. Well, I did it for everything except sagas. I'm leaving it here in case of someone wants to create something like that.

Generate components index (--export-folder)
$ generate-index "src/components/*/*" --output "src/components/index.js" --export-folder --watch

Output:

export Atom from './atoms/Atom'
export Button from './atoms/Button'
...

So we can use it as we do today:

import { Atom, Button } from 'components'
Generate actions index (--export-all)
$ generate-index "src/store/*/actions.js" --output "src/store/actions.js" --export-all --watch

Output:

// is this possible?
export * from './modal/actions'
export * from './post/actions'
...

So we can use it as we do today:

import { modalShow } from 'store/actions'
Generate middleware index (--export-default-array)
$ generate-index "src/store/*/middleware.js" --output "src/store/middlewares.js" --export-default-array --watch

Output:

import entities from './entities/middleware'
...
export default [
  entities,
  ...
]

So we can use it as we do today:

import middlewares from 'store/middlewares'

const configureStore = initialState =>
  createStore(reducer, initialState, applyMiddleware(...middlewares))
Generate reducers index (--export-default-object)
$ generate-index "src/store/*/reducer.js" --output "src/store/reducers.js" --export-default-object --watch

Output:

import entities from './entities/reducer'
import modal from './modal/reducer'
import post from './post/reducer'
...
export default {
  entities,
  modal,
  post,
  ...
}

So we can use:

import { combineReducers } from 'redux'
import { routerReducer as routing } from 'react-router-redux'
import reducers from 'store/reducers'

export default combineReducers({
  routing,
  ...reducers,
})
Generate selectors index (--export-all-named)
$ generate-index "src/store/*/selectors.js" --output "src/store/selectors.js" --export-all-named --watch

Output:

export * as entities from './entities/selectors'
export * as modal from './modal/selectors'
export * as post from './post/selectors'
...

So we can use:

import upperFirst from 'lodash/upperFirst'
import * as modules from 'store/selectors'

export default Object.keys(modules).reduce((rootSelectors, moduleName) => {
  const fromName = `from${upperFirst(moduleName)}`
  const moduleSelectors = modules[moduleName]
  const getState = (state = {}) => state[moduleName] || {}

  rootSelectors[fromName] = Object.keys(moduleSelectors)
    .filter(key => typeof moduleSelectors[key] === 'function')
    .reduce((object, key) => {
      const selector = moduleSelectors[key]
      object[key] = (state, ...args) => selector(getState(state), ...args)
      return object
    }, {})

  return rootSelectors
}, {})

And import it as we do today:

import { fromEntities, fromPost } from 'store/selectors'

fromEntities.getDetail(state, fromPost.getDetail())

@declanelcocks
Copy link

@diegohaz You're using a library for this generate-index? Just asking as I couldn't find it

@diegohaz
Copy link
Owner Author

@declanelcocks No, that's just an interface idea for a possible library.

@declanelcocks
Copy link

Ah ok, makes sense. I suppose some of the code from the current index.js file could be re-used to create the logic for generating all the import statements.

@declanelcocks
Copy link

declanelcocks commented Mar 22, 2017

@diegohaz Also noticed that even doing export Atom from './atoms/Atom' (actually had to use { default as Atom } for every component in an index.js file, I will still end up with one big chunk with the entire app code.

@nealoke
Copy link

nealoke commented May 20, 2017

What I ended up doing was this for my atoms, molecules and organisms as they never (in my case) clash:

const req = require.context('.', true, /\.\/(atoms|molecules|organisms)\/[^\/]+\/index\.js/);

req.keys().forEach((key) => {
	const componentName = key.replace(/^.+\/([^/]+)\/index\.js/, '$1');
	module.exports[componentName] = req(key).default;
});

And for my containers I just added containers.js to my components directory which does the following. Same goes for pages and templates:

const req = require.context('.', true, /\.\/containers\/[^\/]+\/index\.js/);

req.keys().forEach((key) => {
	const componentName = key.replace(/^.+\/([^/]+)\/index\.js/, '$1');
	module.exports[componentName] = req(key).default;
});

This way you can do:

import { Atom, Molecule, Organism } from 'components';
import { Container } from 'components/container';
import { Page } from 'components/pages';

@ghalex
Copy link

ghalex commented Jul 11, 2017

How is this working with jest ? I get this error when I try to run my tests:
TypeError: require.context is not a function

@diegohaz
Copy link
Owner Author

@ghalex require.context doesn't work with jest. That's one of the reasons we mock all components except the one being tested.
https://github.com/diegohaz/arc/blob/master/private/jest/componentsMock.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants