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

Simplify import for complex components #2706

Closed
unional opened this issue Dec 29, 2015 · 23 comments
Closed

Simplify import for complex components #2706

unional opened this issue Dec 29, 2015 · 23 comments

Comments

@unional
Copy link

unional commented Dec 29, 2015

Will you consider to support:

import Card from 'material-ui/lib/card';
import List from 'material-ui/lib/list';

let MyCard = () => (
  <Card>
    <Card.Header ... />
    <Card.Media ... />
    <Card.Title ... />
  </Card>
);

let MyCard = () => (
  <List>
    <List.Item>
  </List>
);

along with the current way:

import Card from 'material-ui/lib/card/card';
import CardActions from 'material-ui/lib/card/card-actions';
import CardHeader from 'material-ui/lib/card/card-header';
import CardMedia from 'material-ui/lib/card/card-media';
import CardTitle from 'material-ui/lib/card/card-title';

I remember seeing this recommendation on react.io, but couldn't find the source.

@newoga
Copy link
Contributor

newoga commented Dec 30, 2015

@unional We are currently looking into approaches to simplifying, but don't think this approach is currently planned or has been discussed. The only current component that is doing something like this to the best of my knowledge is <AutoComplete/>.

@oliviertassinari @alitaheri What do you think about this? The only concern I could imagine having is would implementing this approach mean that if you import Card it in effects imports all related components. Maybe that's okay for components like that though when you are likely to use them.

Related to #2679

@alitaheri
Copy link
Member

@newoga I'm afraid doing this will force import many unused components leading to increased size of the final build. @oliviertassinari What do you think about this?

I believe we should actually remove the component that are using this approach too. If we want to support orthogonality.

@unional
Copy link
Author

unional commented Dec 30, 2015

If the component are or can be used orthogonally, then of course they should be separate. This only applies to sub-component/sub-section-component that are meant to be used under the main component.

Another (better) example would be Table vs TableBody etc.

@alitaheri
Copy link
Member

@unional I see. might not be a bad idea after all. @oliviertassinari what do you think?

@newoga
Copy link
Contributor

newoga commented Dec 30, 2015

@alitaheri I had same concern as you but am starting to agree that this could be a good approach for some of the more specific use cases. I know react-bootstrap uses this approach for some of their nav stuff too.

@newoga
Copy link
Contributor

newoga commented Dec 30, 2015

@unional I guess it's worth mentioning that a similar approach to this is using the * syntax for importing...

import * as Card from 'material-ui/lib/card'

let MyCard = () => (
  <Card.Card> 
    <Card.Header ... />
    <Card.Media ... />
    <Card.Title ... />
  </Card.Card>
);

That should work right now.

@oliviertassinari
Copy link
Member

I'm afraid doing this will force import many unused components leading to increased size of the final build.

I agree. For instance, I don't like this line https://github.com/callemall/material-ui/blob/master/src/auto-complete.jsx#L354. It can lead to unused imported files. The three shaking of rollup or webpack has no way to know if the Divider will be used or not.

The solution proposed by @newoga seems to address the issue 👍 .

@alitaheri
Copy link
Member

@oliviertassinari I don't think that solution can be implemented if we plan on flattening the folder structure. I think we should flatten the structure, so every component (no matter how tightly coupled) are imported using the exact same pattern material-ui/lib/XXX

@unional
Copy link
Author

unional commented Dec 30, 2015

IMO AutoComplete should not export MenuItem and Divider as it currently is. When designing API, we should ask ourselves what is the intent:

  1. Here is AutoComplete, as well as AutoComplete.Divider which is a sub-component that can only be used under AutoComplete, or
  2. Here is AutoComplete, as well as AutoComplete.Divider which I provide as I think it would be convenience for you as you would use it in most common scenairos. It is Divider by itself and can be use elsewhere too.

In my world, (2) always lose.
(1) is the proper use case for Table, Table.Body, Table.Row, etc.

@unional
Copy link
Author

unional commented Dec 30, 2015

@newoga , that would work. I would suggest to maintain some form of it after the flattening.

Another way would be:

import {Card, CardHeader, CardTitle} from 'material-ui/lib/card';

@alitaheri
Copy link
Member

@unional Hmmmm... You make some valid points!

@newoga @oliviertassinari I have a proposal:

how about we flatten the ENTIRE structire:

import Card from 'material-ui/lib/Card';
import {CardHeader} from 'material-ui/lib/CardHeader';
import {CardTitle} from 'material-ui/lib/CardTitle';

but still provide a way to require all the related components through re-exports:

import {Card, CardHeader, CardTitle} from 'material-ui/lib/card-all'; // or whatever

@unional
Copy link
Author

unional commented Dec 31, 2015

@alitaheri , you may draw some similarity from lodash modern:
https://github.com/lodash/lodash/tree/3.10.1-es
where lodash.js => material-ui or material-ui/index
and each component has its own export, such as array.js => Card.js

import _ from 'lodash'; // Get everything
import array from 'lodash/array'; // Get only array related

@alitaheri
Copy link
Member

@unional That's how it is right now. 😁

@unional
Copy link
Author

unional commented Dec 31, 2015

I see. Do you see the tree-shaking in webpack would become a standard? I'm using jspm, not webpack.

@alitaheri
Copy link
Member

@unional I will eventually. But I don't think we should rely on it. If it doesn't work as exactly as expected the output file can become devastatingly big!

@newoga
Copy link
Contributor

newoga commented Dec 31, 2015

@alitaheri I think standard ES6 module syntax is all we need. For example, another approach that currently works is:

// assuming Card is exported by default and CardHeader and CardTitle is normal exported
import Card, {CardHeader, CardTitle}  from 'material-ui/lib/Card';

That would eliminate our need to have some material-ui/lib/card-all import path.

Also, nothing is stopping us from still exporting everything at root material-ui path. I think supporting both is fine and not too hard. 😄

@alitaheri
Copy link
Member

@newoga 😱 😱 I take my suggestion back it was stupid 😆. Good idea 👍 👍

I really like it 😁

@oliviertassinari I think we should follow this, do you agree?

@oliviertassinari
Copy link
Member

Well, I think that the best way is to let people use

import {Card, CardHeader, CardTitle} from 'material-ui/lib/card';

or

import * as Card from 'material-ui/lib/card'

In order to do so, I guess that we just need this in our card/index.js

import Card from './card';
import CardHeader from './card-header';
import CardTitle from './card-title';
import CardMedia from './card-media';
import CardText from './card-text';
import CardActions from './card-actions';
import CardExpandable from './card-expandable';

export {Card};
export {CardHeader};
export {CardTitle};
export {CardMedia};
export {CardText};
export {CardActions};
export {CardExpandable};

@alitaheri
Copy link
Member

That can't happen if we decide to flatten the folder structure.

P.S. you don't have to re-export the default, this is equivalent and simpler:

export {default as Card} from './card';

@unional
Copy link
Author

unional commented Dec 31, 2015

How about separating export and implementation?
Implementations still sit under material-ui/lib/**, while exporting at top level:

material-ui/TextField.jsx:

export {TextField as default} from './lib/TextField.jsx';

material-ui/Table.jsx:

import {Table} from './lib/Table.jsx';
import {TableBody} from './lib/TableBody.jsx';

export default Table;
export {TableBody};

// Or
export {Table as default} from './lib/Table.jsx';
export * from './lib/TableBody.jsx'

Another point to discuss is whether to do default or named export, i.e. export default class TextField ... vs export class TextField ... in the implementation.

In the framework that I wrote, I prefer to only do named export for all internal files. This is because each file may have multiple exports (while violating SRP, some functions are either too small, too parallel in feature (e.g. variations for the same responsibility), too couple, or exposed to facilitate testing), and the consumers of the files many time need to open the file to find out which is exported as default.


To sum everything up in one sentence:

Only do export default for the module's single point of entry

In material-ui, each orthogonal component is a module.

@yongxu
Copy link
Contributor

yongxu commented Jan 4, 2016

AutoComplete.Divider was a bad design. When I initially wrote this component ThemeManager was still entangled with most components and the everything was imported at once.

Thinking about the old way react addons got imported, I quite like the solution @unional proposed:

import Card from 'material-ui/lib/card';
import Card from 'material-ui/lib/card/card'; //no extra

@unional
Copy link
Author

unional commented Jan 13, 2016

Fyi, tree-shaking may be available in jspm.
systemjs/builder#442

@newoga
Copy link
Contributor

newoga commented Mar 9, 2016

I'm going to close this discussion (it's planned for 0.15.0), if anyone has anymore thoughts we can continue in #2679.

@newoga newoga closed this as completed Mar 9, 2016
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

5 participants