[compiler] Only run validations with env.logErrors on outputMode: 'lint' #35216
Conversation
Summary: These validations are not essential for compilation, with this we only run that logic when outputMode is 'lint' Test Plan: Update fixtures and run tests
|
|
||
| ```javascript | ||
| import { c as _c } from "react/compiler-runtime"; // @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects | ||
| // @validateNoSetStateInEffects @enableAllowSetStateFromRefsInEffects @outputMode:"lint" |
There was a problem hiding this comment.
this file compiles w/o error, why do we need to change the output mode here? I would expect that we'd only set the outputMode explicitly for fixtures that break w/o it
There was a problem hiding this comment.
but then the test would not be checking anything right? Because the code that could trigger the error doesn't run.
Like if some change in ValidateNoSetStateInEffects suddenly made this test error out we wouldn't notice
There was a problem hiding this comment.
Nice, thanks for putting this up! Should save a bit of time on compilation since the no-derived-computations-in-effects rule is fairly complex.
For the fixtures, let's only change them to outputMode:"lint" if strictly necessary, ie to show that problematic code does report an error? Or were you thinking that we want to use lint mode anyway to test that the file doesn't error?
This might be an indication that our tests should do what we do in playground: compile in both modes and show a combined view of the output. I'm curious for @mofeiZ's take.
Yeah, that's why I also changed the files that don't error |
| } else if (env.config.validateNoDerivedComputationsInEffects) { | ||
| validateNoDerivedComputationsInEffects(hir); |
There was a problem hiding this comment.
should we change this mode to work similarly? do we want to keep this version of the validation around?
josephsavona
left a comment
josephsavona
left a comment
There was a problem hiding this comment.
This makes sense. As a follow-up, I do think we should seriously consider updating snap to work like playground, ie run both normal build mode and lint mode for each fixture, unless the fixture opts into special behavior. That way we don't need to have multiple versions of the same input to test build vs lint behavior (which i've already done in a few places)
But this makes sense to land, thanks for working on it!
Summary:
These validations are not essential for compilation, with this we only run that logic when outputMode is 'lint'
Test Plan:
Update fixtures and run tests