Open diegohaz opened 7 years ago
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.
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.
@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.
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.
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.
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.
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?
https://github.com/gajus/create-index
This might be useful?
@advance512 Am I correct in assuming it's an automated way of doing this below?
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.
@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.
@advance512 Out of curiosity did you try to implement something like the above?
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.
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.
@diegohaz You're using a library for this generate-index
? Just asking as I couldn't find it
@declanelcocks No, that's just an interface idea for a possible library.
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.
@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.
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';
How is this working with jest
? I get this error when I try to run my tests:
TypeError: require.context is not a function
@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
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 https://github.com/diegohaz/arc/issues/128#issuecomment-281914665
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.