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

[codemod] Prepare the import path breaking change #11249

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 5, 2018

Prepare the import path flattening breaking change, stuff like:

-+import Table, {
-  TableBody,
-  TableCell,
-  TableFooter,
-  TablePagination,
-  TableRow,
-} from 'material-ui/Table';
+import TableRow from 'material-ui/TableRow';
+import TablePagination from 'material-ui/TablePagination';
+import TableFooter from 'material-ui/TableFooter';
+import TableCell from 'material-ui/TableCell';
+import TableBody from 'material-ui/TableBody';
+import Table from 'material-ui/Table';

People should be able to upgrade without any pain with this codemod. This is definitely more verbose. Things to take into account:

  1. It's a simple pattern to learn. People won't need to go back and forth with the documentation to learn the import paths 💭.
  2. People bundle size will decrease 🚀.
  3. In an ideal world, we would import everything from the root module and tree sharking would be taken care of for us. This change doesn't matter in this world ☮️.
import {
  Table,
  TableBody,
  TableCell,
  TableFooter,
  TablePagination,
  TableRow,
} from 'material-ui';

For #9673

@oliviertassinari oliviertassinari added the new feature New feature or request label May 5, 2018
@oliviertassinari oliviertassinari self-assigned this May 5, 2018
@oliviertassinari oliviertassinari force-pushed the code-mode-path-migration branch 2 times, most recently from 275b40f to f4ed944 Compare May 5, 2018 17:26
@oliviertassinari oliviertassinari mentioned this pull request May 5, 2018
@oliviertassinari oliviertassinari force-pushed the code-mode-path-migration branch from f4ed944 to 0c7b82e Compare May 5, 2018 17:37
@oliviertassinari
Copy link
Member Author

Here is the output running the script on our documentation:
$ jscodeshift -t packages/material-ui-codemod/src/v1.0.0/import-path.js docs/

capture d ecran 2018-05-05 a 19 39 57

@oliviertassinari oliviertassinari merged commit c9846bd into mui:v1-beta May 5, 2018
@oliviertassinari oliviertassinari deleted the code-mode-path-migration branch May 5, 2018 18:04
import GridListTile from 'material-ui/GridListTile';
import GridList from 'material-ui/GridList';
import CircularProgress from 'material-ui/CircularProgress';
import MuiLinearProgress from 'material-ui/LinearProgress';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small glitch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a stress test. Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the other imports are prefixed, so...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People might be aliasing the modules:
import foo from 'bar' or import { foo as bar} from 'baz'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get why MuiLinearProgress rather than just LinearProgress, but whatever.

Copy link
Member Author

@oliviertassinari oliviertassinari May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It tests that the following transformation is done:

input

import { LinearProgress as MuiLinearProgress } from 'material-ui/Progress';

output

import MuiLinearProgress from 'material-ui/LinearProgress';

@oliviertassinari
Copy link
Member Author

@tomscholz What's your concern here?

@tomscholz
Copy link

tomscholz commented May 6, 2018

The reaction was more of a "no-brainer", that's is also why I removed it. My concern was that, like you motioned, this way it's more verbose. After completely reading your initial comment, I now agree with the changes :) Sorry for the confusion 😅

-Tom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants