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

Implement the experimental Hooks proposal behind a feature flag #13968

Merged
merged 19 commits into from Oct 29, 2018

Conversation

@sebmarkbage
Member

sebmarkbage commented Oct 25, 2018

Hooks let you use all React features (such as state and lifecycle) without writing a class.

We have presented the Hooks proposal at React Conf 2018. This pull request implements all proposed built-in Hooks behind a feature flag, both for the client and the server renderer.

This is a very early time for Hooks and this proposal is experimental. We're merging it into master now so we can iterate on it. It is turned on only in the 16.7 alpha release so that you can try it and provide your feedback.

We have been using this code for a month in production so while the APIs are not stable, we don't expect the implementation to contain major bugs.

Please keep the feedback and discussion contained in the Hooks RFC (and not in this PR) so that everyone can discuss it in a single place without checking multiple GitHub threads.

@sizebot

This comment has been minimized.

sizebot commented Oct 25, 2018

Details of bundled changes.

Comparing: 5045763...5045763

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@NE-SmallTown

This comment has been minimized.

Contributor

NE-SmallTown commented Oct 25, 2018

I'm so sorry to say this in this thread but I notice this:

committed on 6 Sep

Oh guys, the last time you do this is the Suspense. 😄

Great work !

-------- I will remove this comment if it causes spam

@aweary

aweary approved these changes Oct 25, 2018

😍

@acdlite

Wait wait I want to approve it, too!

@jamiebuilds

Hold up, but I also want to approve it

@TrySound

Great stuff

@sophiebits

wow, great idea

@philipp-spiess

⚓️

]);
});
it('flushes passive effects even if siblings schedule a new root', () => {

This comment has been minimized.

@philipp-spiess

philipp-spiess Oct 26, 2018

Collaborator

I wonder how you came up with all of those edge cases. 😉

This comment has been minimized.

@gaearon

gaearon Oct 28, 2018

Member

Ran into bugs in the first implementation by trying it on the product code.

@Kovensky Kovensky referenced this pull request Oct 26, 2018

Merged

Add type definitions for the proposed React Hooks API #30057

8 of 8 tasks complete
false,
'Calling useContext(Context.Consumer) is not supported, may cause bugs, and will be ' +
'removed in a future major release. Did you mean to call useContext(Context) instead?',
);

This comment has been minimized.

@Kovensky

Kovensky Oct 27, 2018

Interesting; this also means that you can't use the usual const { Provider, Consumer } = React.createContext() pattern with hooks.

This comment has been minimized.

@trueadm

trueadm Oct 27, 2018

Contributor

There’s no reason to use that pattern anymore with hooks.

This comment has been minimized.

@Andarist

Andarist Oct 28, 2018

Contributor

@trueadm actually this could be useful for libraries wanting to hide their providers 🤔

This comment has been minimized.

@gaearon

gaearon Oct 28, 2018

Member

It's plausible one of returned values would be a Hook itself in the future.

@gaearon

seems okay

acdlite and others added some commits Sep 5, 2018

Initial hooks implementation
Includes:
- useState
- useContext
- useEffect
- useRef
- useReducer
- useCallback
- useMemo
- useAPI
Defer useEffect until after paint
Effects scheduled by useEffect should not fire until after the browser
has had a chance to paint. However, they should be fired before any
subsequent mutations.

Also adds useMutationEffect and useLayoutEffect. useMutationEffect fires
during the host update phase. useLayoutEffect fires during the post-
update phase (the same phase as componentDidMount
and componentDidUpdate).
Add support for hooks to ReactDOMServer
Co-authored-by: Alex Taylor <alexmckenley@gmail.com>
Co-authored-by: Andrew Clark <acdlite@fb.com>
Make sure deletions don't stop passive effects
Before the fix, the passive effect in the test is never executed.

We were previously waiting until the next commit phase to run effects. Now, we run them before scheduling work.
The Lost Effect, chapter 2
Previously, flushPassiveEffects (called by scheduling work) would overwrite rootWithPendingPassiveEffects before we had a chance to schedule the work.
The Lost Effect, chapter 3
wow, writing code is hard

sophiebits and others added some commits Oct 13, 2018

Skip updating effect tag when skipping effect
For example, if you have `useEffect(..., [])`, there's no need to set .effectTag to `Update | Passive` on updates.
Disable hook update callback (2nd arg to setState/dispatch)
I put the feature behind a feature flag, along with a warning, so
we can phase it out in www.
Clear effect tags from a fiber that suspends in non-concurrent mode
Even though we commit the fiber in an incomplete state, we shouldn't
fire any lifecycles or effects.

We already did this for classes, but now with useEffect, the same is
needed for other types of work, too.
import invariant from 'shared/invariant';
import warning from 'shared/warning';
type BasicStateAction<S> = S | (S => S);

This comment has been minimized.

@TrySound

TrySound Oct 29, 2018

Contributor

This is more sound I think. In current case flow will always refine any value and function params and return value won't be checked. I had the same problem in powerplug.

type BasicStateAction<S> = (S => S) | S;

This comment has been minimized.

@acdlite

acdlite Oct 29, 2018

Member

Yeah we found the same thing when testing at FB. Forgot to update in the upstream brach. :)

}
export function useState<S>(
initialState: S | (() => S),

This comment has been minimized.

@TrySound

TrySound Oct 29, 2018

Contributor

Here the same problem. (() => S) is unreachable.

@acdlite acdlite force-pushed the sebmarkbage:hooks branch from 46952bc to ddbfe2e Oct 29, 2018

@acdlite acdlite force-pushed the sebmarkbage:hooks branch from 0d46487 to 5045763 Oct 29, 2018

@acdlite

This comment has been minimized.

Member

acdlite commented Oct 29, 2018

I'm merging this PR now so that we can continue to iterate on changes to Hooks in separate PRs. The feature flag for Hooks will remain disabled (so they won't be accessible in releases) until we come to a decision on the RFC.

@acdlite acdlite merged commit 5045763 into facebook:master Oct 29, 2018

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
@sebmarkbage

This comment has been minimized.

What was the reason this uses the dynamic switch? It is always preferable to use the static one.

This comment has been minimized.

Member

acdlite replied Nov 22, 2018

I think I thought I would use a GK for dogfooding, before I realized that made no sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment