We used to terminate the search on host nodes, and then use the nested unmount algorithm.
However this means we didn't unmount portals inside the host nodes.
We will probably need to restructure this somehow but for now I just added a recursive call to unblock myself.
* Remove output field
The idea was originally that each fiber has a return value. In practice
most of what we're modelling here are void functions and we track side
effects instead of return values.
We do use this for coroutines to short cut access to terminal yields.
However, since this can be nested fragments we end up searching the tree
anyway. We also have to manage this in transferOutput so it ends up being
as expensive. Maybe we can save some traversal for updates when coroutine
branches bail out but meh.
* Unmount child from the first phase of a coroutine
This removes updateContainer and instead uses the regular child mutation
methods to insert into the root container and portals.
Since we're no longer clearing out the container DOM in updateContainer
we have to do that manually during initial mount. This now works on a
document and one of the tests end up unmounting the body when you render
into the document so I had to work around that bit since we don't yet
properly support rendering into the document root.
I tried to add a temporary check for the correct state in https://gist.github.com/spicyj/338147e989215b6eeaf48a6ce2d68d93 and run our test suites over it, but none of our existing test cases would have triggered that invariant. The new one I added does.
We're walking backwards up to the root to find the parent so that we can
propagate events further up to nested React parents. If we don't find a
root, that means that the tree was unmounted and we shouldn't send any
events to it.
This was supposed to fix an issue in refs-test but instead it revealed that
this strategy is broken.
The problem is that Placement effect is not sufficient when the insertion
is completed since the effect flag is reset then and the previous tree
has no effects in it to indicate that this is the wrong tree.
If there is no child there is nothing to tell us that this was the
workInProgress branch. Therefore, we need to look at the other branch
if there are children in it to see if *that* was the workInProgress branch.
We've already been warning for mutating styles so now we'll freeze them
in DEV instead. This isn't as good because we don't have more specific
warnings than the generic error that doesn't even fire in sloppy mode.
This lets Fiber pass the same tests.
This used to be done at the end of the transaction but I made it synchronous.
For this to work it needs to be applied after we have already set the .type
property since it is read by inputValueTracker.
We need this to safely extract the current event listeners from the props.
Unfortunately, we are still not safe to use the return pointer since it
may not point to the current tree when the Fiber is reused. So this is not
fully done yet.
* Use the public ReactDOMServer in tests
We need this because it runs the injection.
* Let Fiber skip react data attributes and comments in SSR tests
This markup is testing implementation details rather than behavior.
* [Fiber] Add ReactDOMFiber.unstable_createPortal()
While #8368 added a version of `ReactDOM.unstable_renderSubtreeIntoContainer()` to Fiber, it is a bit hacky and, more importantly, incompatible with Fiber goals. Since it encourages performing portal work in lifecycles, it stretches the commit phase and prevents slicing that work, potentially negating Fiber benefits.
This PR adds a first version of a declarative API meant to replace `ReactDOM.unstable_renderSubtreeIntoContainer()`. The API is a declarative way to render subtrees into DOM node containers.
* Remove hacks related to output field
* Update all Jest packages to 17.x, cache babel-jest transforms
* Remove the caching
Looking at the other builds it doesn't seem to actually be that necessary. The bottleneck is executors, not build time.
* Remove unnecessary package, fix fiber test runner
* Regenerate yarn lockfile
I forgot that the normal properties route doesn't do this. We also have
to make sure that the order is 1) insert children (options), 2) set
multiple 3) update the options.
* Update ESLint to 3.10.2
Also pull in fbjs for extending properly, per @zpao. This also disables consistent-return, which has about 80 failing cases in React currently. If we'd like to turn this back on, we should do it separately and fix all the call sites properly (rather than just adding 'return undefined;' everywhere, which adds no value.
Fixes to all existing lint errors plus an update for yarn.lock to follow.
* Update yarn.lock after the eslint update.
* Fix all new eslint failures
Unfortunately I had to add three eslint-disable-next-line instances. All have explanations inline.
* Switch Travis to use yarn instead of npm
This traverses parent based on the type of internal instance it is passed.
If it is a Fiber it may have to traverse multiple steps until it finds a
HostComponent.
This will allow us to use the event system with Fiber.
This adds precaching to ReactDOMFiber. I.e. adding a handle from the DOM
node to the internal Fiber. This means that we need to expose an internal
handle to the reconciler.
We use duck typing to figure out if it is a Fiber or Stack instance.
The new failing tests are failing because this is now able to actually
fire events onto Fibers and then the result of those events are
incorrect where as they were ignored before.
* Reapply Check for event listener in props instead of bank (#8192)
This reverts the previous revert.
* Don't throw on falsy event listeners
Some code relies on passing null. This restores the earlier behavior.
This matches Stack behavior.
Had to rewrite a test that used Simulate because Fiber doesn't support events yet.
Also changed tests for componentDidUpdate() since Fiber intentionally doesn't pass prevContext argument.
It is error-prone to push and pop context in begin or complete phases because of all the bailouts. Scheduler has enough knowledge to see if pushing is necessary because it knows when we go inside or outside the tree.
I'll plan to change all of our console.error and component-tree expects to expectDev. It's a little annoying that we need to make sure tests don't throw (see my change to normalizeCodeLocInfo) but any alternative would seem to require two separate test runs or a much more cumbersome syntax.
* Handles risky callbacks on setState. Fixes#8238
* Updates try-catch to cover `callback` when context is not present.
* Updates code to trapErrors instead of swallowing them.
* Fixes flow errors
* Incorporates review comments
* Traps only the first error.
Removes `callbackWasCalled` and updates fiber tests.
I came up with a contrived case of where nested controlled events could
fire within the same batch - but on different targets.
I think we came to the conclusion that controlled values typically cannot
use preventDefault so it is ok that they don't flush until after the event
has finished. So therefore we accumulate a queue of all the nested targets
within a batch and then restore state on all of them.
I'm still skeptical that this is the correct way to do controlled values.
The reason we have to do them in a single event loop is because when you
type, the sequence of values that get accepted or not can matter. I wonder
if there is a scenario we can come up with where you can fire multiple
inner events in an event loop and end up with batching causing problems.
This effectively just reimplements asap again but with no allocations for
a single target and no closure allocations.
* Use _hostContainerInfo to track test renderer options
The transaction is not available when unmounting or updating the
instance, so we track it using _hostContainerInfo
* Throw if hostContainerInfo is not populated in getPublicInstance
* Linting fixes
* Remove transaction from ref lifecycle code path
We don't need to pass the transaction around anymore since we store the
test options on _hostContainerInfo instead
* Remove unused argument
If a controlled target fires within another controlled event, we should not
restore it until we've fully completed the event. Otherwise, if someone
reads from it afterwards, they'll get the restored value.
Not super happy with this particular solution.
* took codes.json from the 15-dev branch
* fixed react:extract-errors task in gulpfile
* generated error codes
* Revert "generated error codes"
This reverts commit b8f3aeed9d8f0d469edd5f6623fa6090930594d8.
* Added a README for the error code system
This reverts commit a878c3056d.
The previous stuff that this builds on didn't successfully land fully. Since I
want to measure this in isolation, I'll revert this for now.
This just reads events from the props instead of storing them in a
listener back.
I had to rewrite a bunch of tests to cover this model.
I removed the tests that just test the adding and removing over listeners
since there is no equivalent behavior anymore.
This reverts the implementation in 33325ad009.
I didn't mean to merge the implementation since it is incorrect and incomplete.
I meant to just merge the unit test.
* Ensure that we're listening to all onChange dependencies for controlled inputs
When there is no onChange event handler, we should still listen to it to
ensure controlled values get reset.
There is warning associated with this but the prod behavior should still be
respected.
* Rerun tests
* Use explicit pass for restoring controlled state instead of asap
This gets rid of the first class ReactUpdates.asap scheduling. Instead of
scheduling a first class function to invoke later, I introduce an explicit
phase for restoring controlled state after dispatching events.
We assume that the deepest event target is the thing that needs its state
restored.
* Drop special cases for onChange in wrappers
We're now not using anything special about onChange handling in the
wrappers. Yay!
* Attempt to fix ReactDOMTextComponent test in Fiber
Obviously, some tests pass but the reconciliation is still incomplete on this in Fiber.
Those changes just ignore reconciliation Comments while asserting (this fixes 2 tests already) but now shows that Fiber lacks some reconciliation features.
* Transmit a parentInstance to commitTextUpdate
This is done in case a Node#normalize() was performed on the parent instance and that the TextInstance got desolidarized from the parent. We provide the parent known by the parent Fiber to reattach the DOM node in this case.
* Added a more complex case around split nodes
This test does not assume one split node substitution alone but also some non-split nodes.
* Fiber supports split text nodes
This is done by suppressing nodes appearing during the split. this is based by comparing against the old value known by the Fiber.
* Simplify siblings cleaning in commitTextUpdate
No need to assert instanceof Text since the length comparator will work correctly.
* Ensure non-text nodes won't be removed
* Fix flow for Fiber Text reconciliation additions
Mostly define types as optionals. Respects the strict DOM API around Node & Element.
* Append at any stack stage only if necessary
The direct parent of the TextNode fiber may not contain the host node. We have to go through the hierarchy. However, since this work may be expensive, we only do it if absolutely necessary!
* Use tag & ReactTypeOfWork to find an HostComponent
* Addfailing scenario when running Node.normalize()
It happens when non-text-elements are added in the mix. @sebmarkbage seems to have an idea on how to fix it. This is just a repro of the bug!
* Register failing/passing fiber tests
This gets rid of the global flag on if something has listened to onSelect
and instead reads the isListening map if all the events are covered.
This is required if we want to attach events locally at roots.
Could be slower perf wise to handle events. An alternative solution would
be to attach a special flag on the listener map for the document so we
don't have to check the full dependency list.
However, my favorite solution would be to just eagerly attach all event
listeners (except maybe wheel). Then we don't have to do any of this stuff
on a per element basis.
* Print a warning in fiber for lacking render method
* Reject initial non-object state in Fiber
Rejects Arrays, Strings & Numbers. Allows nulls.
* Centralize fiber checks on ES6 Classes
* Add classic & instance property checks on fiber
- check the absence of getInitialState & getDefaultProps as methods
- check the absence of propTypes & contextTypes at instance-level
* Convert to normalized React `warning` calls
* Support lifecycle typo detection in fiber
* Get the complete warnings from the existing code
* Only check classInstance once
Avoid rechecking while resuming
* Can warn component while resuming but only once
This is achieved by tracking the warning state in the Fiber structure.
* Remove warning deduplication
* Factor name retrieval in a getName helper
* Use invariant instead of throw
* Read inst.state only once for the invariant
* Fix condition on the instance state
* Register failing/passing fiber tests
* Refactor scheduling functions and introduce Task priority
There was lots of duplication across all the scheduling functions. I
think we're far enough along that we can start trying to clean some
stuff up.
Also introduces a new priority level provisionally called Task priority.
This is for work that completes at the end of the current tick, after
the current batch of work has been committed. It's different from
Synchronous priority, which needs to complete immediately.
A full implementation of Task priority will follow. It will replace
the current batching solution.
* Implement Task priority
Task priority is similar to Synchronous priority. Both are flushed in
the current tick. Synchronous priority is flushed immediately (e.g. sync
work triggered by setState will flush before setState exits), where as
Task is flushed after the current batch of work is committed.
Currently used for batchedUpdates and nested sync updates. Task should
also be used for componentDidUpdate/Mount and error boundary work. I'll
add this in a later commit.
* Make error boundaries use Task Priority
I have all but one error fixed. Not sure how tricky the last one is,
but I'm cautiously optimistic. Pushing to show my current progress.
* Remove recursion from handleErrors
Changed the algorithm of handleErrors a bit to ensure that boundaries
are not revisited once they are acknowledged.
* Add incremental error boundary test
Discovered an edge case: a noop error boundary will break in incremental
mode unless you explicitly schedule an update.
* Refactor error boundaries in Fiber
* Simplify trapError() calls
The existing logic was written before we had a proper error handling system.
* Remove unnecessary flags
* Prevent performTaskWork recursion
* Be stricter about preventing recursion
Before, this code was bailing out after encoutering a null child because it thought it was out of old children. Seen when reconciling [null, <span />, <Image />] with [null, <span />, <Image />] in XUISelector.
Instead of scheduling individual callbacks to asap, we schedule one and
then do all the work in that one.
I'm doing this for architectural refactoring reasons.
Nevertheless, I'm adding a contrived unit test that this fixes. :)
* Make test more complete
* Add a failing test for scheduling in reverse order
It hangs forever because we don't clear next pointer when unscheduling a root. Therefore, when it's scheduled again, it brings all its previous chain with it, potentially creating a cycle.
* Clear the next pointer when unscheduling a root
Fixes a potential infinite cycle when we reschedule a root.
* Add new tests to Fiber test tracker
* Introduce facts-tracker
We want to be able to track various things like number of files that are flowified, number of tests passing with Fiber...
This introduces a tool that lets you do that. The API is very simple, you execute the script with a list of tuples [key value] and it's going to create a `facts` branch and put a txt file for each fact and values over time.
```
node scripts/facts-tracker/index.js \
"flow-files" "$COUNT_WITH_FLOW/$COUNT_ALL_FILES"
```
Test Plan:
This is tricky to test because Travis only exposes the private variables (github token) when it processes a committed file and not on branches. The reason is that otherwise anyone could send a pull requests that does `echo $GITHUB_TOKEN` and steal your token.
Given this constraint, I did all the work using two of my repos:
- https://github.com/vjeux/facts-tracker
- https://github.com/vjeux/facts-tracker-test
and am sending this pull request that should work as is /fingers crossed/, but we won't be able to test it out until it is committed.
Note that once this lands, I'm going to kill those two repos.
* Update with all the suggested changes
* Branch on a flow type in travis.yml
* Use $GITHUB_TOKEN
* properly escape it
Jest 15 has just been released and is way more awesome: http://facebook.github.io/jest/blog/2016/09/01/jest-15.html
This pull request updates jest from version 12 to 15. Right now there's a fix in jest around Symbol that hasn't been released yet, so this will break CI and cannot be merged it. But once it ships by the end of the day, we'll be good to go :)
See comments inline for all the changes I've done.
We currently write all our tests against the DOM implementation.
I need a way to run the Fiber tests against it. But I don't want
to take on any package dependencies on Fiber modules yet.
There's a problem with jest right now where you can't globally
mock modules that already exist. So I have to add a global call
to jest.mock.
Luckily we already have a way to test the useCreateElement paths
using a feature flag. I won't activate this flag in travis until
it passes, but the idea is to run all three variants in travis.
I'm not sure that invoking rAF and rIC synchronously is the best
way to test this since it doesn't capture the backwards
compatibility aspect. I.e. the fact that people might be relying
on the synchronous nature in real apps too. It's a start.
Ideally, jest would have these built-in.
Previously, the extract-components script would create the same number of layers of composites as the page it captures, but it would output a new class for each time any composite is used (since we don't want to replicate all the component logic).
I changed the script to output a single type for each type in the input -- and each generated component takes an index for which output it should return. This should be closer to how the original code behaves, especially with respect to VM function call lookups where the amount of polymorphism makes a difference.
I re-recorded the benchmarks with the new scripts. They run significantly faster:
```
Comparing old.txt (control) vs new.txt (test)
Significant differences marked by ***
% change from control to test, with 99% CIs:
* ssr_pe_cold_ms_jsc_jit
% change: -41.73% [-43.37%, -40.09%] ***
means: 39.3191 (control), 22.9127 (test)
* ssr_pe_cold_ms_jsc_nojit
% change: -44.24% [-46.69%, -41.80%] ***
means: 45.8646 (control), 25.5764 (test)
* ssr_pe_cold_ms_node
% change: -45.61% [-47.38%, -43.85%] ***
means: 90.1118 (control), 49.0116 (test)
```
This is probably in part due to the changes here, but also the page I captured has changed somewhat in the meantime and there seem to be slightly fewer components in the hierarchy, so they're not really comparable. But going forward we can use this benchmark which should be more accurate. I also included an identical copy that uses stateless functional components so we can test optimizations to those later.
Test Plan: Changed the preprocessor to log the output of babel.transform and saw
```
var _jsxFileName = 'src/isomorphic/modern/element/__tests__/ReactJSXElementValidator-test.js';
```
in the resulting output, instead of an absolute path.
Running this is left as an exercise for the reader, since my measure.py isn't designed for this at present. But something like this might work:
```diff
diff --git a/scripts/bench/measure.py b/scripts/bench/measure.py
index 4cedf47..627ec97 100755
--- a/scripts/bench/measure.py
+++ b/scripts/bench/measure.py
@@ -79,15 +79,12 @@ def _measure_ssr_ms(engine, react_path, bench_name, bench_path, measure_warm):
if (typeof React !== 'object') throw new Error('React not laoded');
report('factory_ms', END - START);
- globalEval(readFile(ENV.bench_path));
- if (typeof Benchmark !== 'function') {
- throw new Error('benchmark not loaded');
- }
+ globalEval("bm = (function(){" + readFile("bench-createclass-madman.js") + "})");
+ bm();
var START = now();
- var html = React.renderToString(React.createElement(Benchmark));
- html.charCodeAt(0); // flatten ropes
+ bm();
var END = now();
- report('ssr_' + ENV.bench_name + '_cold_ms', END - START);
+ report('cc_' + ENV.bench_name + '_cold_ms', END - START);
var warmup = ENV.measure_warm ? 80 : 0;
var trials = ENV.measure_warm ? 40 : 0;
@@ -119,7 +116,7 @@ def _main():
return 1
react_path = sys.argv[1]
- trials = 30
+ trials = 60
sys.stderr.write("Measuring SSR for PE benchmark (%d trials)\n" % trials)
for i in range(trials):
for engine in [
@@ -132,7 +129,7 @@ def _main():
sys.stderr.flush()
sys.stderr.write("\n")
- trials = 3
+ trials = 0#3
sys.stderr.write("Measuring SSR for PE with warm JIT (%d slow trials)\n" % trials)
for i in range(trials):
for engine in [
```
Works at least on a CentOS 7 machine after running `sudo yum install
webkitgtk webkitgtk-devel`.
The only globals you get are `print` and `PerfCounters`. No `console` nor the other globals provided by the `jsc` command-line tool (load, readFile, etc) though they're probably not hard to implement.
You can disable the JIT by setting the environment variable `JSC_useJIT=false`.
Test Plan:
```
~/local/react/scripts/perf-counters$ make
~/local/react/scripts/perf-counters$ build/jsc-perf <(echo 'PerfCounters.init(); var a = PerfCounters.getCounters().instructions; print("moo"); var b = PerfCounters.getCounters().instructions; print(b - a);')
moo
72182
~/local/react/scripts/perf-counters$
```
You could make the argument that this should be optional, but it doesn't
work without it so we might as well just enforce it.
Makes `jest` work by default.
Turns out jest is _incredibly_ slow at resolving require paths like `require('fbjs/lib/foo')`. Like several milliseconds per require. Really adds up when all our files require `invariant` and `warning`. Here's a temporary hack to make things fast again.
Test Plan:
```
npm test src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js
```
has a self-proclaimed runtime of ~8 seconds now instead of ~35 seconds.
This reworks a few things in building and distributing React. The biggest change is using fbjs to share dependencies with other libraries. We're also using Gulp for some build steps.
Created a .mailmap file with all of the associations, then used
git + perl to create the AUTHORS file. In theory these should all get
picked up by npm.
I used ABC order so it would remain unbiased and automatable. I wish we
could go back and fill out the history or at least fix the commits we
have from CommitSyncScript, but oh well.
This also includes the script I used to automate this process in the
future.
Christopher Chedeau
