-
Notifications
You must be signed in to change notification settings - Fork 432
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
chore: add dev:million-lint
command
#8032
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
30be234
to
7e5974a
Compare
No changes to documentation |
⚡️ Editor Performance ReportUpdated Fri, 13 Dec 2024 12:36:29 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
Component Testing Report Updated Dec 13, 2024 12:34 PM (UTC) ❌ Failed Tests (4) -- expand for details
|
7e5974a
to
7414464
Compare
7414464
to
9d075f4
Compare
@@ -83,3 +83,6 @@ yalc.lock | |||
## Documentation Report | |||
scripts/docs-report.md | |||
|
|||
# Temporary data collected by Million Lint | |||
**/.million/store.json | |||
dev/test-studio/.react-compiler-bailout-report.json |
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.
Committing this to git would cause all sorts of annoying merge conflicts on PRs that do perf work, that for example makes components no longer bail out. Or when eslint-plugin-react-compiler
and babel-plugin-react-compiler
itself changes what cases it bails and supports as we bump deps.
? { | ||
target: '18', | ||
sources: (filename) => { | ||
if (filename.includes('node_modules')) { |
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.
This is the default behavior of sources
, since we're overriding it we have to ensure we don't accidentally try running the compiler on non-src files from npm.
vite(viteConfig: UserConfig): UserConfig { | ||
const reactProductionProfiling = process.env.REACT_PRODUCTION_PROFILING === 'true' | ||
|
||
return { | ||
...viteConfig, | ||
plugins: millionLintEnabled | ||
? [ | ||
require('@million/lint').vite({ |
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 to use it as a regular import at the top originally.
But for some reason, if you have this line top-level:
import Million from '@million/lint'
then sanity build
hangs forever
@@ -17,7 +17,8 @@ | |||
"check:deps": "pnpm --recursive --parallel exec depcheck", | |||
"check:format": "prettier . --check", | |||
"check:lint": "turbo run lint --continue -- --quiet", | |||
"check:react-compiler": "eslint --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 79 .", | |||
"check:react-compiler": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 77 .", | |||
"report:react-compiler-bailout": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [error,{__unstable_donotuse_reportAllBailouts:true}]' --ignore-path .eslintignore.react-compiler -f ./scripts/reactCompilerBailouts.cjs . || true", |
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 reason this command has __unstable_donotuse_reportAllBailouts:true
is because we want to make sure Million Lint runs on all components that are not optimized by the compiler.
Running the eslint compiler plugin reports 77 issues atm that it considers actionable.
Actionable means its bailing because of a problem with the code, for example reading a ref.current
during render (which has always been unsafe, and the reason it's not impacting us atm is because we're over-rendering).
There are a lot of cases where the compiler skips a component that's valid. For example it doesn't fully support TypeScript "satisfies" syntax, here it bails, while here it works.
These type of issues aren't flagged without the __unstable_donotuse_reportAllBailouts
option to avoid leading people to refactor code in ways to only please the current version of the compiler, instead of what's best for code readability and for the humans working in the project.
In this case we don't really care or show the bailed out components that aren't actionable, we just gobble it up in a JSON file to setup mutually exclusive source filters between Million Lint and React Compiler.
That way we get intel on which of the components that don't have react errors, but for whatever reason isn't compiled, is worth looking into manually memoizing in the mean time.
Description
This PR sets up a script that run Million Lint on the Test Studio locally:
million.mov
There's a lot to say on this topic, and if performance work interests you and you have a hot beverage ready, expand the
Verbose information
toggle at the end of the PR. The PR will otherwise be as brief and easy to review as possible 🙌With Million Lint we can:
useEffect
.What to review
Have a look on the diff, as I'm calling out what to review as my own review comments.
Please mark them as resolved once you've seen them, and feel free to ask direct follow up questions there and then resolve once you're satisifed with my answer 😌
Testing
Until Million supports running in production you'll only able to run it locally, and AFAIK the IDE extension is only available on VS Code for now.
git clone
,pnpm install
and runpnpm dev:million-lint
.You can look at
dev/test-studio/.react-compiler-bailout-report.json
if you wonder exactly what files are instrumented and will have intellisense available.Why can't we just run Million on every file?
Since we currently have to disable React Compiler if we want to run Million on everything, it significantly increases renders and thus render data.
This isn't the first time we've had a problem like this. Our very own @runeb has official React contributor status after he sent a fix that allowed us to run the React DevTools profiler on more than just a few seconds in the Studio.
On an M1 Pro 32GB RAM MacBook Pro you'll barely able to view a simple document list in Structure Tool, open a document, or type to fast in the search input and it dies.
On a M4 Max 128GB RAM MacBook Pro you get quite a bit further, and can type into fields for a few minutes before it caves. Typing into a Portable Text field kills it almost instantly.
Notes for release
N/A this is purely a monorepo addition, only to the test studio. It doesn't affect what we publish to npm with
sanity
or@sanity/vision
in any way.Verbose information
How Million and React Scan compares to React DevTools
Million builds on top of React Scan, and integrates its performance profiling data with VS Code.
The benefit of React Scan, over the regular React Profiler in React DevTools, is that it instruments and traces how often hooks, props and state change over time and how it leads to cascading rerenders.
React DevTools can tell you what happened on each render frame, and is enough when you deal with performance issues where a single render is very slow.
In our codebase we have very few problems like that (they happen at scale, such as very large documents, large amount of fields, massive amount of editors working in the same document, huge Portable Text fields, those kind of scenarios). The general jank and slowness in the Studio is due to the "death of a thousand cuts". We have constant, cascading renders, paired with memoization that triggers garbage collection events as the memoization can't survive unstable props, state and context values. React DevTools isn't well equipped to help us here, what we need is the ability to track changes and counters over time in order to find the needles in our haystack.
How does React Compiler come into play?