-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Bump jscodeshift to ^0.15.2 #34680
Bump jscodeshift to ^0.15.2 #34680
Conversation
Netlify deploy previewhttps://deploy-preview-34680--material-ui.netlify.app/ Bundle size report |
956dc0c
to
a0f02a0
Compare
a0f02a0
to
7078000
Compare
7078000
to
17a2955
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output looks wrong, it generates duplicate brackets. cc @siriwatknp
17a2955
to
4f0dfcf
Compare
4f0dfcf
to
5d62c80
Compare
3b4351e
to
f424e73
Compare
125097f
to
876f52a
Compare
876f52a
to
dbc323f
Compare
dbc323f
to
1d6295f
Compare
1d6295f
to
8c31857
Compare
8c31857
to
4c30610
Compare
183fd28
to
c2febc9
Compare
c2febc9
to
865372d
Compare
I think we should merge this dep update for v6. I assume that most projects have code formatting tools like prettier. I am updating the tests. |
Edited/Blocked NotificationRenovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above. ⚠ Warning: custom changes will be lost. |
How about in |
|
packages/mui-codemod/codemod.js
Outdated
@@ -162,6 +162,11 @@ function run(argv) { | |||
|
|||
runJscodeshiftTransform(codemod, files, flags, argv._); | |||
runPostcssTransform(codemod, files); | |||
if (!flags.skipPrettier) { | |||
childProcess.spawnSync('prettier', ['--write', `prettier --write $(git diff --name-only)`], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a bit heavy to spawn an extra process, wouldn't it be possible as a jscodeshift transformer?
import * as prettier from 'prettier';
export default async function transformer(file) {
const prettierConfig = await prettier.resolveConfig(file.path);
return prettier.format(file.source, prettierConfig);
}
Almost feels like those APIs were made for each other 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We need to add prettier as a dependency, so using
npx
will be a bit slower, right? - Adding this requires a lot of changes to the expected files. I try to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- there could be a one time cost in some cases, looks like prettier total install size is about 50% of jscodeshift total install size (total size = package + dependencies). I'm not sure npx install time is something we should optimize for here, ideally this will be run at most once ever over the lifetime of a project.
- I suppose you can still make it optional with a
skipPrettier
flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- ideally this will be run at most once ever over the lifetime of a project
If the codemod release a new version, isn't another installation is required?
I suppose you can still make it optional with a skipPrettier flag
Yes, it can be skipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried timing the installation of jscodeshift
alone and also jscodeshift
and prettier
together and the difference seems to be insignificant (<10ms)
packages/mui-codemod/codemod.js
Outdated
} else if (!flags.skipPrettier) { | ||
try { | ||
childProcess.execSync('prettier --write $(git diff --name-only)', { | ||
stdio: 'inherit', | ||
}); | ||
} catch (error) { | ||
// silentyly ignore errors | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, with the CSB build, it works fine. @Janpot what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I don't think we should do it this way. I suppose the idea of ignoring all errors is to ignore when prettier
is not installed. But this will also silence any other useful error. For instance, in large projects this will potentially generate a large amount of file names, which can result in failures in the shell as there are limits to the command length. We've ran into this ourselves with code infra with our prettier
command and this is the reason why we now use a tool that runs prettier programmatically.
I think we should run prettier programmatically on every file if:
- there is a straightforward way to run it e.g. through a jscodeshift transformer
- there is a straightforward way to turn it off with a flag
- there is a straightforward way to turn it off when no prettier config exists in the project
If any of these 3 conditions is impossible, I'd consider not adding prettier formatting at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which can result in failures in the shell as there are limits to the command length
I think it's an edge case because the prettier only runs on changed files. I used to run codemod on several large projects (backstage is one of them) but I've never found a case where file changes go beyond hundreds before.
Anyway, in case there is an error, --skip-prettier
can be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is to not add prettier at first and see the feedback. I will remove it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up here after running the deprecations codemod in our docs. I had some files present the double parens issue. In my case, it was easy to bypass running prettier manually. It wasn't that bad. I wouldn't complain if prettier was ran automatically, maybe as an opt-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should at least add a warning for this in the codemod readme.
closes #36173
This PR contains the following updates:
^0.13.1
->^0.15.2
0.11.5
->0.11.11
Release Notes
facebook/jscodeshift (jscodeshift)
v0.15.2
Compare Source
Fixed
v0.15.1
Compare Source
Changed
@babel/plugin-proposal-private-methods
in worker (#568, @sibelius)Fixed
v0.15.0
Compare Source
Changed
Fixed
v0.14.0
Compare Source
Added
defineSnapshotTestFromFixture
test util (#471, @shriuken)renameTo
filters for Babel 6+ node types (#412 and #504, @elonvolo and @henryqdineen)childNodesOfType
to JSX traversal methods (#415, @j13huang)Changed
--help
to be listed in an order other than alphabetically, so they can instead be grouped thematically (#507, @elonvolo)j
shortcut in test utils (#515, @no23reason)Configuration
📅 Schedule: Branch creation - "on sunday before 6:00am" in timezone UTC, Automerge - At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about these updates again.
This PR has been generated by Mend Renovate. View repository job log here.