-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Comments
Yes please. I have spent hours looking in the docs to know the exact path of the components for importing. |
Note that there are two possible approaches to this:
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:
|
@mbrookes I would give a strong +1 for option n°1. |
@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. |
@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. |
@oliviertassinari I've updated https://github.com/mbrookes/material-ui/tree/flat-src-dir/src per current suggested structure. Only |
@mbrookes I like that. I'd also lobby for moving |
@kgregory svg-icons I agree. test-utils are documented though: https://material-ui.com/guides/testing/ |
@mbrookes indeed they are, my bad! |
All 👍, no 👎, I think it's a go! |
We're just waiting for #9561 to be merged, rather than creating rebase hell! |
@mbrookes I was waiting on getting approval for the scope package before starting this. Batching the effort will be more efficient for everybody. |
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? |
@justinryder I believe that the earlier we perform the breaking changes the better. We might release a code mode for this one. |
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. 😄 |
would situations like this be effected?
|
-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' |
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? |
@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. |
Although it should be stressed that, just as it does now, the first will import everything. |
@kgregory gotcha, works for me |
@tony-kerz In theory, but my understanding is that it doesn't work very well, possibly due to |
I would be very sad to loose I think the grouping helps the readability quite a lot. |
@Pajn however this would mean requiring Table (which is always required) would still include all other components if no tree shaking is enabled. 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 |
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.
CircularProgress
.Progress/CircularProgress
.It's unclear to me why
CircularProgress
is under aProgress
folder or whyRadio
isn't under aSwitch
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.
Going back to the example, we would move
src/Progress/CircularProgress.js
tosrc/CircularProgress/CircularProgress.js
. People will be able to use the following import:Let me know what you think about this proposal! The issue was raised by @neoziro.
The text was updated successfully, but these errors were encountered: