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

v1.0.0 Codemod skipping all files #11369

Closed
1 task done
andokai opened this issue May 14, 2018 · 31 comments
Closed
1 task done

v1.0.0 Codemod skipping all files #11369

andokai opened this issue May 14, 2018 · 31 comments
Labels
status: waiting for author Issue with insufficient information

Comments

@andokai
Copy link
Contributor

andokai commented May 14, 2018

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

It should update source files as described.

Current Behavior

It skips all the source files.

$ find src -name '*.js' -print | xargs jscodeshift -t node_modules/@material-ui/codemod/lib/v1.0.0/import-path.js
Processing 78 files...
Spawning 3 workers...
Sending 26 files to free worker...
Sending 26 files to free worker...
Sending 26 files to free worker...
All done.
Results:
0 errors
0 unmodified
78 skipped
0 ok
Time elapsed: 1.852seconds

Your Environment

Tech Version
Material-UI 1.0.0-rc.0
React 16.3.2
Node.js 10.1.0
NPM 6.0.1
@fzaninotto
Copy link
Contributor

Same problem here

@fzaninotto
Copy link
Contributor

I believe the renaming of the imports must be done before passing the codemod

@andokai
Copy link
Contributor Author

andokai commented May 14, 2018

Ah yes, I see what you mean.

@andokai andokai closed this as completed May 14, 2018
@fzaninotto
Copy link
Contributor

fzaninotto commented May 14, 2018

Even though I renamed the imports, only a few files are changed. I believe that the codemod only changes lines like

import { Card, CardContent } from '@material-ui/core';

But not lines like

import Card, { CardContent } from '@material-ui/core/Card';

Which is a pity since the latter is the recommended way according to the doc...

@fzaninotto
Copy link
Contributor

@andokai you shouldn't close the issue, it's far from resolved IMO

@andokai andokai reopened this May 14, 2018
@andokai
Copy link
Contributor Author

andokai commented May 14, 2018

It certainly still required manual intervention on my part. The following wasn't transformed for me.
import ListSubheader from '@material-ui/core/List/ListSubheader';

@oliviertassinari
Copy link
Member

Which is a pity since the latter is the recommended way according to the doc...

It should be working. It's the previous pattern used by all our demos. The demos were migrated with the script. However, I have just realized that the test for the codemod has been transformed. I'm restoring them so we can move forward: #11376.

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label May 14, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2018

Can anyone provide a source file example that isn't transformed but should be?

@TheMoonDawg
Copy link
Contributor

That's very strange. I also had this issue until I renamed my imports from:
import Card, { CardContent } from 'material-ui/Card
to
import Card, { CardContent } from '@material-ui/core/Card'

Looking at my files after the script, everything looks good. I will post back if something looks out of place.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2018

@MoonDawg92 It can be configured:

https://github.com/mui-org/material-ui/blob/9ec2fcf5d8349e8f074fe5e07fc4018f66b90845/packages/material-ui-codemod/src/v1.0.0/import-path.js#L74-L75

@fzaninotto
Copy link
Contributor

@oliviertassinari react-admin@v2.0.0-RC3 would be a good test project (the code to migrate is in packages/ra-ui-materialui/).

By the way, the codemod only helps with... code. That means that all the text files (documentation) have to be updated by hand.

@fzaninotto
Copy link
Contributor

I can confirm that the codemod updated some files after I renamed the import path to @material-ui/core, but many imports were skipped (I'd estimate about 60%) by the codemod.

@oliviertassinari
Copy link
Member

@fzaninotto
Copy link
Contributor

it's already migrated to rc.0 in master (refs marmelab/react-admin#1824) (by hand)

@lookfirst
Copy link
Contributor

Can anyone provide a source file example that isn't transformed but should be?

All my typescript files?!

@jacobweber
Copy link

Looks like ListSubheader is also a problem in the docs:

import ListSubheader from 'material-ui/List/ListSubheader';

@oliviertassinari
Copy link
Member

All my typescript files?!

@lookfirst I can't do much about jscodeshift not supporting TypeScript :/. I have added a comment, facebook/jscodeshift#180 (comment).

@psamim
Copy link

psamim commented May 15, 2018

All files are skipped using parser=flow for jscodeshift.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 15, 2018

@psamim Is your source code using @material-ui/core?

@psamim
Copy link

psamim commented May 15, 2018

@oliviertassinari I have this line in the code:

import { withStyles } from 'material-ui/styles';

@oliviertassinari
Copy link
Member

oliviertassinari commented May 15, 2018

@psamim You need to change the imports from material-ui to @material-ui/core before using the codemod. The codemod is only about flattening the imports.

Flattening the imports is a long and tedious work, so we automated it. Renaming material-ui to @material-ui/core is trivial.

@psamim
Copy link

psamim commented May 15, 2018

Aha, thanks.

@pavanshinde47
Copy link

jscodeshift -t <color-imports.js> --importPath='mui/styles/colors' --targetPath='mui/colors'

Use this command and put the codmod script path, your source path and change the import and target path accordingly .

@Forfold
Copy link

Forfold commented May 17, 2018

We weren't able to get the codemod to work on our project. It kept erroring out saying "error at token export" or something similar yesterday. We ended up writing our own, which handles material-ui to @material-ui/core too, however it doesn't handle all of the edge cases that Olivier tackled; just what we needed for our project. Feel free to look here if you think it might help your work: https://gist.github.com/Auchindoun/34703c57237e228583aee4ca333d6372

We ran it with jscodeshift --parser babylon -t ./codemods/material-core.js ./app

@bluepeter
Copy link

Thank you @Auchindoun!

@sacdallago
Copy link
Contributor

Just for everyone else coming here, wondering like me what this means:

I believe the renaming of the imports must be done before passing the codemod

it actually means doing a global search+replace on the src from material-ui to @material-ui/core and then running the Codemod.

I suggest making this crystal clear here: https://github.com/mui-org/material-ui/tree/master/packages/material-ui-codemod#import-path . For someone who has never used codemod before, reading that doc, this is not the behavior I expected. I was holding back on doing a global search+replace because I couldn't believe that a script that would flatten imports wouldn't as well rename the imports, if ultimately the latter is a prerequisite for the former (and yes, I spend a good amount of time thinking I was stupid :D ).

@oliviertassinari
Copy link
Member

I suggest making this crystal clear here

@sacdallago I'm all in with you 💯. Is that something you would like to contribute to the project? :)

sacdallago added a commit to sacdallago/material-ui that referenced this issue May 30, 2018
Quick fix in ref to mui#11369 (comment) : suggest users to perform search+replace
@sacdallago
Copy link
Contributor

sacdallago commented May 30, 2018

@oliviertassinari 🙏 I'm super glad for your (and everyone else's) contributions on this project. I was truly just speaking about a cosmetic change on the documentation (which I have proposed in #11647), rather than a full blown script.

@Auchindoun from what I see, you basically have done exactly this pre-flattening step (aka: find+replace) in your script. That could be a nice addition to https://github.com/mui-org/material-ui/blob/master/packages/material-ui-codemod/src/v1.0.0/import-path.js

oliviertassinari pushed a commit that referenced this issue May 30, 2018
Quick fix in ref to #11369 (comment) : suggest users to perform search+replace
@oliviertassinari
Copy link
Member

@sacdallago comment should greatly reduce the confusion. I'm closing.

acroyear pushed a commit to acroyear/material-ui that referenced this issue Jul 14, 2018
Quick fix in ref to mui#11369 (comment) : suggest users to perform search+replace
@devuxer
Copy link

devuxer commented Mar 27, 2019

@oliviertassinari,

So am I understanding correctly that these migrations do not work for TypeScript projects?

If so, can you please advise on how to go about migrating a TypeScript project?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 27, 2019

@devuxer I have seen people using the codemod with TypeScript: facebook/jscodeshift#180 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Issue with insufficient information
Projects
None yet
Development

No branches or pull requests