Fix ReactiveDependencies rule #3
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…k#13959) - Partial revert of facebook#13732 - Fixes facebook#13958.
|
@gaearon I'm assuming this PR isn't going to run CI since it's in a different repo... What do you want me to do with it? |
|
Want to just submit both original commit and further work on top as your own PR? I’ll close mine. Let’s just keep first commit credited to Caleb. |
* Simplify imports in ReactChildFiber * Import type first in ReactCurrentFiber * Simplify imports in ReactFiberBeginWork * Simplify imports in ReactFiberScheduler * Simplify import in ReactFiberTreeReflection * Simplify import in ReactFiberUnwindWork * Remove repeated import * Fix imports from ReactFiberExpirationTime * Master imports in ReactFiberBeginWork
People are probably gonna do this all the time.
Mostly to catch this:
```js
useEffect(async () => {
// ...
return cleanup;
});
```
Is this too restrictive? Not sure if you would want to do like
```js
useEffect(() => ref.current.style.color = 'red');
```
which would give a false positive here. We can always relax it to only warn on Promises if people complain.
…acebook#14003) * chore: don't rely on jest fake timers scheduling real timers * re-add one part not working with Jest 23
… 16.4.2 and 16.5.0. Fails in Edge in some scenarios. (facebook#14095)
Replaced 'var' with 'const'
Check for existence of `setTimeout` and `clearTimeout` in the runtime before using them, to ensure runtimes without them (like .NET ClearScript) do not crash just by importing `react-dom`.
* Add debug tools package * Add basic implementation * Implement inspection of the current state of hooks using the fiber tree * Support useContext hooks inspection by backtracking from the Fiber I'm not sure this is safe because the return fibers may not be current but close enough and it's fast. We use this to set up the current values of the providers. * rm copypasta * Use lastIndexOf Just in case. I don't know of any scenario where this can happen. * Support ForwardRef * Add test for memo and custom hooks * Support defaultProps resolution
console.error.apply() fails in IE9, but I verified this works (and it works everywhere else too). :)
This is required to use lazy. Test Plan: * Verified lazy works on a real world use case (shows spinner, shows real content). * Verified that if I change the primary content's styles to have `display: 'none'` then it never appears (i.e., the code in `unhide` reads the styles successfully)
…g primary children (facebook#14083) * Avoid double commit by re-rendering immediately and reusing children To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit. * Add a failing test for remounting fallback in sync mode * Add failing test for stuck Suspense fallback * Toggle visibility of Suspense children in mutation phase, not layout If parent reads visibility of children in a lifecycle, they should have already updated.
The `enableHooks` feature flag used to only control whether the API was exposed on the React package. But now it also determines if the dispatcher and implementation are included in the bundle. We're using hooks in www, so I've switched the feature flag to `true` in the www build. (Alternatively, we could have two feature flags: one for the implementation and dispatcher, and one for exposing the API on the React package.)
Setting to null isn't correct; setting to '' is. I opted to use dangerousStyleValue for consistency with the main path that we set things. Fixes facebook#14114. Test Plan: Verified setting to '' works in Chrome and IE11. (Setting to null works in Chrome but not in IE11.)
* Resolve defaultProps for Lazy components * Make test fail again * Undo the partial fix * Make test output more compact * Add a separate failing test for sync mode * Clean up tests * Add another update to both tests * Resolve props for commit phase lifecycles * Resolve prevProps for begin phase lifecycles * Resolve prevProps for pre-commit lifecycles * Only resolve props if element type differs * Fix Flow * Don't set instance.props/state during commit phase This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different. This can mess with resuming. * Keep setting instance.props/state before unmounting This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error.
Adds a check to the existing fuzz tester to confirm that the props are set to the latest values in the commit phase. Only checks componentDidUpdate; we already have unit tests for the other lifecycles, so I think this is good enough. This is only a redundancy.
* Recover from errors with a boundary in completion phase * Use a separate field for completing unit of work * Use a simpler fix with one boolean * Reoder conditions * Clarify which paths are DEV-only * Move duplicated line out * Make it clearer this code is DEV-only
Fix comment typo
I don't think "array or iterator" is adding anything, and it may well be confusing, especially since this is one of the first and most common warnings that devs see.
* Separate_Object_Is_Polyfill
gaearon
force-pushed
the
reactive-rule
branch
from
bf7c57b
to
b5bac9d
Compare
gaearon
force-pushed
the
reactive-rule
branch
from
ade085e
to
dc9bb2e
Compare
* react-debug-tools accepts currentDispatcher ref as param * ReactDebugHooks injected dispatcher ref is optional * Support custom values for custom hooks * PR feedback: 1. Renamed useDebugValueLabel hook to useDebugValue 2. Wrapped useDebugValue internals in if-DEV so that it could be removed from production builds. * PR feedback: 1. Fixed some minor typos 2. Added inline comment explaining the purpose of rollupDebugValues() 3. Refactored rollupDebugValues() to use a for loop rather than filter() 4. Improve check for useDebugValue hook to lessen the chance of a false positive 5. Added optional formatter function param to useDebugValue * Nitpick renamed a method
Fixes facebook#14583 Using `new Set([iterable])` does not work with IE11's non-compliant Set implementation. By avoiding this pattern we don't need to require a Set polyfill for IE11
* Added Flow type to keep hooks dispatchers in-sync
* Add ESLint rule playground * Update index.js * Update index.js
gaearon
force-pushed
the
reactive-rule
branch
from
dc9bb2e
to
8797fa6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm interested in how parts of this might have been working in the past because it seemed to rely on things that ESLint does not do.
In particular, ESLint doesn't have a NodePath type like Babel so they assign
node.parentas the parent node. But for some reason they don't do this everywhere. So you end up in scenarios like we had here where you don't have access to the parent node.I was not able to find a way to get the parent node from ESLint's
context.getScope().references. So I was forced to do the worst possible thing and search through the entire AST for the node we're looking for.I've optimized this search as much as possible. It's similar to a binary search except we're working with a generic tree. The search doesn't know anything about the AST though, it just looks for anything with a
{ type: string, ... }and assumes that has the same basic interface as other nodes do in ESLint.