Skip to content
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

[Fiber] Umbrella for remaining features / bugs #7925

Closed
sebmarkbage opened this issue Oct 10, 2016 · 10 comments
Closed

[Fiber] Umbrella for remaining features / bugs #7925

sebmarkbage opened this issue Oct 10, 2016 · 10 comments

Comments

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Oct 10, 2016

This is an umbrella issues for remaining fiber issues. More can be found by running the unit tests with the useFiber flag in ReactDOMFeatureFlags turned on

See #8830 for additional tasks beyond the scope of the initial Fiber release

Phases 1–6

Phase 1: Infrastructure

Phase 2: Smaller / Initial Tasks

Phase 3: Larger Tasks

Phase 4: Uncovered Bugs

Phase 5

Phase 6: Unit tests and known bugs push


Phase 6.1: 15.5 Release

  • Warn (throw?) when doing an update on a container that was manually emptied outside of React (in stack, this mounted a brand-new tree; in fiber, it tries to apply an update and usually fails) [@keyanzhang, @sebmarkbage took over this] (blocks beta)
  • Extract propTypes into a separate npm package. Get prop-types package name. [@acdlite]
  • Unit tests: ReactContextValidator-test -- Pass the correct previous context to componentDidUpdate (or deprecate feature and remove test). [@bvaughn] Pass prevContext param to componentDidUpdate #8631
  • [fb] Better fix to ReactART.Text.
  • Update warning when calling propTypes directly to refer user to checkPropTypes API
  • Docs updates for test utils and removed add-ons [@flarnie]

Phase 6.2: React Native Fiber

  • Reimplement Shallow Renderer without Stack dependencies [@lelandrichardson/@trueadm] Add createComponentMock option to test renderer #8982
  • RN: ensure we keep all important invariants (such as that text must be wrapped in <Text>) (does not block anything but we should do this)
  • Ensure that we call the FB specific warning module instead of the fbjs/lib/warning one in RN. [@spicyj] (doesn't block anything OSS, internal to FB)
  • Convert propTypes callers to use checkPropTypes. [@acdlite]
  • Deploy forwarding modules to react/lib/ReactCurrentOwner, react/lib/ReactComponentTreeHook, fbjs/lib/invariant, fbjs/lib/warning, fbjs/lib/ExecutionEnvironment, fbjs/lib/performanceNow, fbjs/lib/emptyObject, fbjs/lib/emptyFunction, fbjs/lib/shallowEqual. So that we can keep using providesModule in this repo until we have flat bundles. Or better yet: Do the inverse and switch to using CommonJS naming convention in www. [@sebmarkbage, @bvaughn]
  • See if we can codemod createStrictPropTypeChecker away and use exactShape instead. (doesn't block OSS release)
  • Release a new version of react-redux whose peer is compatible with 16-alpha. Update RN to use it. Add React 16 as a valid peer dep reduxjs/react-redux#629
  • Enable prepareNewChildrenBeforeUnmountInStack feature flag (one week after landing sync).
  • Codemod Flow errors yielded by properly typing findNodeHandle. (we added types and $FlowFixMe comments; good enough for now)
  • Redesign the host component "type", the type of host component refs and integration with NativeMethodsMixin to be upgrade compatible. [@bvaughn]
  • Make sure instanceProps in ReactNativeComponentTree doesn't leak. (@sebmarkbage, @bvaughn)

Phase 6.3: Flat Bundles/Rollup

  • Make React Perf work with Fiber (ReactPerf-test) [@gaearon] (Decided not to in favor of deeper timeline integration)
  • Unit tests: ReactComponentTreeHook-test [@gaearon]
  • Unit tests: ReactHostOperationHistoryHook-test [@gaearon]
  • Make a list of all internal React modules required by FB code
  • Come up with a better strategy for dealing with DEV code and make sure we don't ship it in prod
  • Change www sync script https://fburl.com/juffnc9d to run rollup using commonjs source
    • Shimming the required files (ex: ReactDOMInjection, warning)
    • Exporting a single bundle that exposes the internals we need
    • Make fowarding modules for ReactDOM/etc as well as internal modules
  • Convert OSS repo to ES6 modules (does not block release; nice-to-have follow up)
  • Make separate dev and non-dev build in one file for www (if (__DEV__) { ... all of React ... } else { ... all of React })
  • Convert OSS repo to rollup
  • Replace current Grunt/Gulp build system with a unified build system (maybe purely using Rollup)
  • Add Closure Compiler with ADVANCED and deal with mangling properly (does not block release; nice-to-have follow up)

Phase 7: Async-Compatible (does not block release)

  • Fix incremental regression (if it is a regression)
  • [fb] Fix resumeMountClassInstance so that newProps gets passed to construct class. Otherwise this.props can be null.
  • RN: Aborting async work or unmounting a tree due to an error leaks native views
  • Add cross-renderer support for portals (necessary for async ART)
  • ReactDOMFiber.render - Return the root instance from render even if priority is deferred. Test and cover incremental cases
  • Decide on replacing priorities with deadlines to fix starvation

Phase 8: Server-Side Rendering

  • What are we doing?
  • Unit tests: ReactRenderDocument-test (SSR) -- Reviving server rendered markup (including rendering into document and shadow DOM containers)

Phase 9: Improvements (does not block release)

@donnieflorence
Copy link

Things that I hope would also be appended to this list of features/bugs:

  1. Fixing context
  2. Making stateless functional components more performant
  3. Learning from inferno to provide stateless function components some lifecycle hooks
  4. Maybe revising/improving reacts legacy build step
  5. Try to cut down react size in the major rewrite

@sophiebits
Copy link
Collaborator

@donnieflorence This is a list of things we need to do to get parity with the existing implementation. After we finish these we might look at adding new features.

@gaearon
Copy link
Collaborator

gaearon commented Nov 8, 2016

How to Help

  1. Watch an intro to Fiber
  2. Read about it here and here
  3. Clone React repo
  4. Run REACT_DOM_JEST_USE_FIBER=1 npm test to run unit tests with Fiber
  5. Pick a failing test and try to fix it (unless it fits into a broader group claimed by someone in the first comment)
  6. Run scripts/fiber/record-tests to verify which tests are now passing with Fiber
  7. Submit a PR!

Note: Contributing is hard right now.

It will get easier once more pieces (e.g. DOM attributes and events) are in place. There will be a long trail of issues where community can really help. But if you're feeling adventurous, you can give it a go. Check out older Fiber PRs for inspiration and pieces of information. Example Fiber "easy" PR: #8156.

iamdustan added a commit to iamdustan/react that referenced this issue Dec 21, 2016
In facebook#7925 there is a task to:

> Ensure we replace errors with invariant calls and they have sensible
> messages

While it is likely a bit premature to begin that work, this script will
make it easier to find and replace the exact call sites to manage.

Usage:
``` sh
$ ./scripts/fiber/find-errors

```

``` sh
$ ./scripts/fiber/find-errors --quiet

```
iamdustan added a commit to iamdustan/react that referenced this issue Dec 21, 2016
In facebook#7925 there is a task to:

> Ensure we replace errors with invariant calls and they have sensible
> messages

While it is likely a bit premature to begin that work, this script will
make it easier to find and replace the exact call sites to manage.

Usage:
``` sh
$ ./scripts/fiber/find-errors

...lots of output...

src/renderers/shared/fiber/ReactFiberScheduler.js#921:
  throw new Error('No error for given unit of work.');

src/renderers/shared/fiber/ReactFiberScheduler.js#940:
  throw new Error('Invalid type of work.');

src/renderers/shared/utils/ReactErrorUtils.js#55:
  throw error;

All done.
Results:
6 errors
0 unmodified
380 skipped
0 ok
Stats:
error: 42
Time elapsed: 4.031seconds
42 throw statements to to convert to `invariant`

```

``` sh
$ ./scripts/fiber/find-errors --quiet

...lots of output...

src/renderers/shared/fiber/ReactFiberScheduler.js#780:
src/renderers/shared/fiber/ReactFiberScheduler.js#785:
src/renderers/shared/fiber/ReactFiberScheduler.js#921:
src/renderers/shared/fiber/ReactFiberScheduler.js#940:
src/renderers/shared/utils/ReactErrorUtils.js#55:
All done.
Results:
6 errors
0 unmodified
380 skipped
0 ok
Stats:
error: 42
Time elapsed: 3.728seconds
42 throw statements to to convert to `invariant`
```
iamdustan added a commit to iamdustan/react that referenced this issue Dec 21, 2016
In facebook#7925 there is a task to:

> Ensure we replace errors with invariant calls and they have sensible
> messages

While it is likely a bit premature to begin that work, this script will
make it easier to find and replace the exact call sites to manage.

Usage:
``` sh
$ ./scripts/fiber/find-errors

...lots of output...

src/renderers/shared/fiber/ReactFiberScheduler.js#921:
  throw new Error('No error for given unit of work.');

src/renderers/shared/fiber/ReactFiberScheduler.js#940:
  throw new Error('Invalid type of work.');

src/renderers/shared/utils/ReactErrorUtils.js#55:
  throw error;

All done.
Results:
6 errors
0 unmodified
380 skipped
0 ok
Stats:
error: 42
Time elapsed: 4.031seconds
42 throw statements to to convert to `invariant`

```

``` sh
$ ./scripts/fiber/find-errors --quiet

...lots of output...

src/renderers/shared/fiber/ReactFiberScheduler.js#780:
src/renderers/shared/fiber/ReactFiberScheduler.js#785:
src/renderers/shared/fiber/ReactFiberScheduler.js#921:
src/renderers/shared/fiber/ReactFiberScheduler.js#940:
src/renderers/shared/utils/ReactErrorUtils.js#55:
All done.
Results:
6 errors
0 unmodified
380 skipped
0 ok
Stats:
error: 42
Time elapsed: 3.728seconds
42 throw statements to to convert to `invariant`
```
@acdlite acdlite mentioned this issue Jan 19, 2017
17 tasks
@stefensuhat
Copy link

stefensuhat commented Feb 3, 2017

will you support bind context like VueJS?. So there is no need bind function manually.

@acdlite
Copy link
Collaborator

acdlite commented Feb 3, 2017

@ssuhat

This is a list of things we need to do to get parity with the existing implementation. After we finish these we might look at adding new features.

#7925 (comment)

@rchanou
Copy link

rchanou commented Feb 3, 2017

@ssuhat I think they explicitly decided against this a long time ago for being too "magical". Using the experimental property initializer syntax is a nice way to deal with it; you can also try a 3rd party plugin like react-autobind.

@acdlite
Copy link
Collaborator

acdlite commented Feb 3, 2017

Aside from being "magical," the problem with autobinding is that there's a performance cost to binding methods, and it's wasteful to autobind the ones that don't need it.

@elyobo
Copy link

elyobo commented Feb 3, 2017

Something akin to Inferno's linkEvent would be good though, skipping binding while allowing data to be passed through. Often I find myself with code in a loop that would benefit from this, e.g.

<ul>
  {things.map(thing => (
    <li key={thing.key} onClick={() => this.doSomething(thing.id)}>{thing.something}</li>
  ))}
</ul>

Being able to drop the binding there would be nice.

@sebmarkbage
Copy link
Collaborator Author

Let's create a separate issue for any further discussions.

@facebook facebook locked and limited conversation to collaborators Feb 3, 2017
@flarnie
Copy link
Contributor

flarnie commented Jul 7, 2017

We just moved remaining open items from 6.1-6.3 into #8854 and for now decided to archive this issue. We can open a new 'umbrella' issue or individual issues for items in phases 7-9 after we finish the 16.0 release.

@flarnie flarnie closed this as completed Jul 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants