Use "dprint" as a code formatter, replacing eslint#2997
Use "dprint" as a code formatter, replacing eslint#2997arbrandes merged 5 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
e4fa76c to
03411df
Compare
|
@arbrandes @brian-smith-tcril @ChrisChV @rpenido @navinkarkera Could I get your opinions on this? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2997 +/- ##
==========================================
- Coverage 95.61% 95.46% -0.16%
==========================================
Files 1378 1378
Lines 32289 32596 +307
Branches 7374 7479 +105
==========================================
+ Hits 30873 31117 +244
- Misses 1352 1410 +58
- Partials 64 69 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4a20819 to
3b6f0c2
Compare
|
My initial reaction was that I'd prefer to stay in one "ecosystem" if possible, so being able to use I first looked into the I completely agree with the comments you left on your The key difference is that
Conclusions
claude conversation summary from explorationoxfmt vs dprint for frontend-app-authoringInvestigating #2997, where Braden proposes replacing eslint ContextThe repo already uses oxlint (replacing eslint) and it's working well. But oxlint doesn't
Both are Rust/WASM-based and fast. oxlint + dprint + stylelint runs in <3s vs 55s with eslint. Braden's oxfmt config was minimalHe only used: {
"printWidth": 120,
"singleQuote": true
}PR branch: open-craft/frontend-app-authoring@braden/oxfmt This didn't take advantage of oxfmt's newer options, particularly Braden's dprint configFrom open-craft/frontend-app-authoring@braden/dprint ( {
"lineWidth": 120,
"indentWidth": 2,
"useTabs": false,
"typescript": {
"quoteStyle": "alwaysSingle",
"jsx.quoteStyle": "preferDouble",
"semiColons": "always",
"trailingCommas": "onlyMultiLine",
"useBraces": "always",
"operatorPosition": "maintain",
"arrowFunction.useParentheses": "maintain",
"module.sortImportDeclarations": "maintain",
"importDeclaration.sortNamedImports": "maintain",
"importDeclaration.preferSingleLine": false,
"exportDeclaration.sortNamedExports": "maintain"
},
"json": {},
"markdown": {},
"excludes": [
"**/node_modules", "**/*-lock.json", "**/dist", "**/coverage",
"jest.config.js", "env.config.*", "example.env.config.*",
"module.config.js", "**/messages.{ts,js}"
],
"plugins": [
"https://plugins.dprint.dev/typescript-0.95.15.wasm",
"https://plugins.dprint.dev/json-0.21.3.wasm",
"https://plugins.dprint.dev/markdown-0.21.1.wasm"
]
}A separate Mapping dprint settings to oxfmtDirect matches
Key oxfmt setting Braden didn't use
Gaps — dprint settings with no oxfmt equivalent
The pattern here is that dprint supports Additionally, oxfmt caps Proposed oxfmt config to test{
"$schema": "./node_modules/oxfmt/configuration_schema.json",
"printWidth": 120,
"singleQuote": true,
"jsxSingleQuote": false,
"semi": true,
"trailingComma": "es5",
"objectWrap": "preserve",
"arrowParens": "always",
"bracketSpacing": true,
"insertFinalNewline": true,
"ignorePatterns": [
"**/node_modules", "**/*-lock.json", "**/dist", "**/coverage",
"jest.config.js", "env.config.*", "example.env.config.*",
"module.config.js"
],
"overrides": [
{
"files": ["src/**/messages.ts", "src/**/messages.js"],
"options": { "printWidth": 320 }
}
]
}Next steps
|
I think it could be, in the future! For now they're focusing on matching prettier, but as I understand, they're open to adding more configuration options and flexibility once they're finished that initial push.
It's funny, Claude's explanation and options changes make sense, but it actually resulted in an even larger diff 😆 |
|
For me the two options I've considered strongly are:
I'm fine with option 2 if folks object to dprint, but so far I think it's looking pretty nice. We can also continue to experiment with just this repo if there's more to learn before deciding. Having been forced to use |
|
@bradenmacdonald, my reactions. As I'm sure you're aware, this kind of situation is very common (specially in the Javascript Land) when attempting to use new tools: A) the new thing is fast, fancy and/or sexy, but doesn't do everything we want, so B) let's add this other even newer, fancier, and sexier tool to the stack... GOTO A. On the other hand, remaining on the same stack forever is not realistic. Eventually the whole industry is going to move on from eslint (and jest, and webpack), and if we don't have a plan to move with it, we'll be left stranded. Which is all to say: I'm glad you're trying this stuff out, but I'm also glad it's contained to this MFE. I'm not going to stand in the way as long as you agree these are experimental: when this codebase is converted to frontend-base later on (likely this year), we're only going to use oxlint/dprint in the Authoring app if it makes sense to do it for all apps (including opening PRs like this across the board). If it doesn't, we'll have to roll it back. Deal? :) |
dprint is actually 6 years old, vs. ~17 months for oxfmt, so I wouldn't call it newer, fancier, or sexier :p But I absolutely get your point. We pay a big cost every time we switch tools, so there has to be a clear benefit. In this case though I think the benefits are clear: oxlint is way, way, way faster than eslint while also being able to run "type-aware" rules, so it can catch things like missing As for dprint, I don't have such a strong opinion, but for the time being we've had pretty minimal code formatting standardization on this repo, and I think the dprint diff looks like a nice step forward.
Yes, that's what I had in mind anyways. Ideally we'd be able to use oxlint+oxfmt by the time that happens, if they're able to add the "maintain" configuration options we need by then. But for now I'd like to try oxlint+dprint just in this repo and see how it goes. I've been very impressed by oxlint so far in our 10 week trial. And if consensus is to use eslint or some other tool, we'll switch over to that with the frontend-base upgrade. |
I agree to try oxlint+dprint. I've also liked how oxlint has performed over the past few weeks. I wish dprint had more settings to lower the diffs in this change, so that the rollback, if it happens, doesn't have too many changes either. I've investigated, and there's not much that can be done beyond what was already done in this PR. @bradenmacdonald thanks for this work! |
To be clear, I'm hoping it doesn't happen, and also that by then we're on |
|
@arbrandes Would you be able to force-merge this, overriding the codecov checks? |
|
Done! |
Description
We have been testing oxlint in the Authoring MFE and it's been working great. It has been able to catch things that our old eslint config totally missed (e.g. missing
keyprops in React array elements), and once we got the config right, I haven't seen any lint violations that eslint would have caught but oxlint missed.However, oxlint doesn't do "formatting", so a number of formatting issues have crept in that eslint would have caught, but oxlint considers out of scope.
Example:
To resolve this, we could use oxfmt, the formatter companion to oxlint. However, when I tried this out, I found that it produced a very large diff and many of the changes made the code less readable: https://github.com/openedx/frontend-app-authoring/pull/2973/changes
The reason for this is that oxfmt is new and is initially targeting prettier compatibility, with more configuration options coming later. But for now it still reflects prettier's major problem:
So, I found another Rust/wasm-based code formatter, dprint, and it seems great! With this PR, I:
I think the diff it generates looks really nice, and in general its changes are clear improvements, unlike my test of oxfmt. You can compare both diffs to see.
Also, both oxlint and dprint are blazing fast, so running oxlint+dprint+stylelint now with
npm run linttakes < 3 seconds to run on the whole codebase. Before, running eslint+stylelint took 55s !Note: I haven't really looked into what
stylelintis nor why we have it, but we can probably replace it with dprint as well. Then the lint would be 1s faster (33%).Private ref MNG-4763