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

[RFC] Should we flatten the folder structure? #9532

Closed
oliviertassinari opened this issue Dec 17, 2017 · 25 comments
Closed

[RFC] Should we flatten the folder structure? #9532

oliviertassinari opened this issue Dec 17, 2017 · 25 comments

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2017

This issue is opened to discuss the following proposal: flattening the folder structure.

The problem

The problem I try to solve is the following. Let's say you want to use the circular progress component. You have to know two things to get the import working.

  1. Know the name of the component: CircularProgress.
  2. Know the import path: Progress/CircularProgress.

It's unclear to me why CircularProgress is under a Progress folder or why Radio isn't under a Switch folder. There are many more examples in the codebase.

The solution

I think that we can save (2.) complexity by flatting the folder structure. We can make the import path predictable.

import COMPONENT_NAME from 'material-ui/COMPONENT_NAME`;

Going back to the example, we would move src/Progress/CircularProgress.js to src/CircularProgress/CircularProgress.js. People will be able to use the following import:

-import CircularProgress from 'material-ui/Progress/CircularProgress';
+import CircularProgress from 'material-ui/CircularProgress';

Let me know what you think about this proposal! The issue was raised by @neoziro.

@oliviertassinari oliviertassinari changed the title [RFC] Flatten folder structure [RFC] Should we flatten the folder structure? Dec 17, 2017
@thangngoc89
Copy link

thangngoc89 commented Dec 17, 2017

Yes please. I have spent hours looking in the docs to know the exact path of the components for importing.

@mui mui deleted a comment from biomassives Dec 17, 2017
@mbrookes
Copy link
Member

mbrookes commented Dec 17, 2017

Note that there are two possible approaches to this:

  1. Flatten the /src dir
  2. Flatten at build time

The first has the advantage of simplicity, once it's done, it's done; but the with disadvantage of a large /src dir list.

The second has added complexity (build-time script), but:

  • Maintains backwards compatibility with current import sytax (single line named import for multiple related components), whilst also allowing flat imports as described above.
  • Maintains an ordered hierarchial /src directory

@oliviertassinari
Copy link
Member Author

@mbrookes I would give a strong +1 for option n°1.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 17, 2017

@mbrookes also, I know you did an experimentation in the direction with https://github.com/mbrookes/material-ui/tree/flat-src-dir/src. It's not exactly what I'm suggesting. I'm suggesting that we keep grouping the tests, TypeScript definitions, source of a single component under the same folder.

@mbrookes
Copy link
Member

mbrookes commented Dec 17, 2017

@oliviertassinari That'll help.

Edit: That should also allow us to maintain backwards compatible imports if desired, as the index.js for the root components can still provide named exports for the sub-components.

@mbrookes
Copy link
Member

@oliviertassinari I've updated https://github.com/mbrookes/material-ui/tree/flat-src-dir/src per current suggested structure. Only index.js and index.d.ts in each base component directory is updated. each new subcomponent directory would still needs it's own index files.

@kgregory
Copy link
Member

@mbrookes I like that. I'd also lobby for moving svg-icons and test-utils to internal.

@mbrookes
Copy link
Member

mbrookes commented Dec 20, 2017

@kgregory svg-icons I agree. test-utils are documented though: https://material-ui.com/guides/testing/

@kgregory
Copy link
Member

kgregory commented Dec 20, 2017

test-utils are documented though: https://material-ui.com/guides/testing/

@mbrookes indeed they are, my bad!

@mbrookes
Copy link
Member

All 👍, no 👎, I think it's a go!

@mbrookes
Copy link
Member

mbrookes commented Jan 1, 2018

We're just waiting for #9561 to be merged, rather than creating rebase hell!

@oliviertassinari
Copy link
Member Author

@mbrookes I was waiting on getting approval for the scope package before starting this. Batching the effort will be more efficient for everybody.

@justinryder
Copy link

From a simplified import perspective, I like this idea, but from a breaking change perspective, I really don't. I get that the stated goal of this prolonged beta period is to allow for breaking changes, but doing things like moving files around is far from a stable change. I've already run into issues with things like the JSS import moving and having the latest documentation show it in a different place than it was in the MUI vesion we were depending on in an older feature branch. I understand that flattening the directory structure will be a developer usability win in the longterm, but can you please at least wait until just before you release v1 to do it to minimize how often early adopters need to resolve breaking changes?

@oliviertassinari
Copy link
Member Author

@justinryder I believe that the earlier we perform the breaking changes the better. We might release a code mode for this one.

@justinryder
Copy link

Oooh, that would be handy. I've never had the chance to use codemod for a dependency update before, but it seems pretty sweet. I'm just glad you've all been doing a great job of documenting breaking changes (and all other changes for that matter) in the release notes so I always have good docs to read through which helps a lot when debugging. 😄

@tony-kerz
Copy link

would situations like this be effected?

import Table, {TableBody, TableCell, TableHead, TableRow, TableSortLabel} from 'material-ui/Table'

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 23, 2018

@tony-kerz

-import Table, {TableBody, TableCell, TableHead, TableRow, TableSortLabel} from 'material-ui/Table'
+import {
+ Table,
+ TableBody,
+ TableCell,
+ TableHead,
+ TableRow,
+ TableSortLabel,
+} from 'material-ui'

or

-import Table, {TableBody, TableCell, TableHead, TableRow, TableSortLabel} from 'material-ui/Table'
+import Table from 'material-ui/Table'
+import TableBody from 'material-ui/TableBody'
+import TableCell from 'material-ui/TableCell'
+import TableHead from 'material-ui/TableHead'
+import TableRow from 'material-ui/TableRow'
+import TableSortLabel from 'material-ui/TableSortLabel'

@tony-kerz
Copy link

in the above case, since the components are strongly related (in the sense that they are so often used together), and if they were nested would appear in the same level, i think the single line import is more intuitive (and tidier), but i get trying to get away from the import path in general.

are you saying that both of the above forms would be supported in the strategy you are considering?

@kgregory
Copy link
Member

kgregory commented Feb 23, 2018

@tony-kerz If the structure were flattened, each component would be its own importable module (second example), but they would also be offered in an index, allowing for the first example.

@mbrookes
Copy link
Member

Although it should be stressed that, just as it does now, the first will import everything.

@tony-kerz
Copy link

@kgregory gotcha, works for me
@mbrookes gotcha, but should proper application of tree-shaking techniques strip unused components from the result bundle?

@mbrookes
Copy link
Member

mbrookes commented Feb 23, 2018

@tony-kerz In theory, but my understanding is that it doesn't work very well, possibly due to withStyles, and tree shaking having to assume the potential for side-effects, so not stripping unused imported components.

@Pajn
Copy link
Contributor

Pajn commented Mar 7, 2018

I would be very sad to loose import Table, {TableBody, TableCell, TableHead, TableRow, TableSortLabel} from 'material-ui/Table'
Would you consider keeping reexports from the "parents" index files even with the flattened folder structure?

I think the grouping helps the readability quite a lot.

@olee
Copy link
Contributor

olee commented May 10, 2018

@Pajn however this would mean requiring Table (which is always required) would still include all other components if no tree shaking is enabled.
However I also think having some kind of index file would be nice to have like

import Table, {TableBody, TableCell, TableHead, TableRow, TableSortLabel} from 'material-ui/TableComponents'

Though it's also an option to just create a reexport file of required MaterialUi components like this:

// maybe add file alias like '@mui/table' in webpack etc. to import it easily
export * from `material-ui/Table`
export * from `material-ui/TableRow`
export * from `material-ui/TableCell`

which you could then import

@olee olee mentioned this issue May 10, 2018
@oliviertassinari
Copy link
Member Author

@olee The plan is to make the codemod match reality #11249 and release v1. We can start the discussion for v2 if you want.

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

8 participants