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

React 16 Umbrella ☂️ (and 15.5) #8854

Closed
gaearon opened this Issue Jan 24, 2017 · 110 comments

Comments

Projects
None yet
@gaearon
Member

gaearon commented Jan 24, 2017

This list might change, just putting it out there as the first draft.
Posting these together since they're related, and we don't plan more 15.x releases after 15.5 anyway.

To put these changes in context, we have a few goals:

(Click to show)

  • For the past several months, we have been working on a rewrite of React codenamed “Fiber”. Initially, it won’t affect public API, but it brings several new features (like #2127 and #2461). Fiber gives us a solid foundation to improve React core in numerous ways. We’ll be talking more about it soon, and we intend to ship it with React 16 by default.

  • To reduce the bundle size, we need to remove the APIs that we don't recommend, such as createClass and React.DOM.* helpers. We intend to warn once about their usage so that you can start removing dependencies on them. They will still be available as separate packages, but we will exclude them from the default build of React 16.

  • We would like to have more control over the bundles so that we can better optimize them. This is why we are considering switching to flat bundles (and thus removing access to React internals in react/lib/* and react-dom/lib/*) in React 16. This will also mean faster compile times by default for users of Webpack and other bundlers, and faster server-side rendering performance.

Here’s a speculative list of changes we think of doing in these releases:

Past Releases

I’ve moved these to the end of this post.

16 Final

TODO: Add any bugs discovered while testing RC.

16 Wishlist


Past Releases

15.5

(Click to show)

  • ReactDOM fixes, if @jquense @nhunzaker @aweary want to release any.
  • We should probably ship a fix for #8487 (#9005)
  • New warning: callback refs on stateless functional components (#8635, #8739)
  • New API deprecations:
    • React.createClass (will be moved to a separate package, and we provide a codemod if you want to update to ES classes)
    • React.createFactory (will be moved to a separate package or removed, it's a one-liner)
    • React.DOM.* factories (will be moved to a separate package, #8356) We forgot to do it. Moving to 15.6
    • React.createMixin (will be removed, #8853) We forgot to do it. Moving to 15.6
    • React.propTypes (will be moved to a separate package)
    • Deprecate React addons #9207
    • Decide how to handle all of this with the UMD addons build
    • Fix this bug #9351

15.6

See #9398.

16 Alpha

(Click to show)

  • Remove deprecations added in 15
    • Stop appending px to strings (#6899)
    • Remove LinkedStateMixin, valueLink and checkedLink (#8165)
    • Remove React.__spread and React.createMixin
    • Move React.createClass to a separate package
    • Move PropTypes to a separate package.
    • Move React.DOM.* factories to a separate package
    • Decide what to do with the addons build
    • If we keep any addons, they need to work regardless of whether React is aliased to a flat bundle or not (which is not the case now)
  • Strip PropTypes checkers in production build (#8066)
  • Re-add warning about calling PropTypes directly (#8080)
  • More ReactDOM fixes, if @jquense @nhunzaker @aweary want to release any.
  • Make a decision on the input fix by @jquense (for now accepted, but follow up work will change this. @flarnie link to issue on related follow up work)

16 Beta

(Click to show)

  • Actually enable fragments (@flarnie verified this)
  • Make error boundaries official (@bvaughn)
    • Choose lifecycle naming
    • Codemod stuff
    • Decide whether to runtime warn for the old name (decided: let's not do it)
  • Fix the issue with invokeGuardedCallback (tracking in #9577, @acdlite is working on this)
  • (This came up internally) Set a hard limit for recursion #9193. (@acdlite)
  • Duplicate React + string refs results in very confusing error (ref is not a function). We should provide a good invariant for that. (#9962, @spicyj @flarnie and @gaearon) (is this Draft issue also about this? facebook/draft-js#296)
  • 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) (@flarnie in #10210)
  • Additional APIs we need in order to start experimenting with async. (@acdlite)
    • React.unstable_AsyncComponent
    • ReactDOM.flushSync(batch)
  • sanity test - use in a CRA app, and check fixtures
  • Messaging
    • Update isfiberreadyyet.com
    • Compose a Tweet

16 RC

(Click to show)

  • [m-l] Pass DOM props through (#10399, latest is: #10416) (@gaearon, @spicyj, @sebmarkbage, @flarnie, @acdlite)
  • Regression: we should warn if object is passed to an event listener prop #10407 (@aweary) (fixed? in #10453 but needs review updated and merged)
  • Make sure input variables (props, state) point to the correct values for each lifecycle. Example where this isn't the case for componentWillMount: https://jsfiddle.net/m3pL3yaj/ (@acdlite) (fixed in #10481)
  • select's onChange event fires with incorrect selectedIndex, value (#10420 @sebmarkbage)
  • How do we ensure weak minifiers don't ship the bundle twice? #9589 (@flarnie, @trueadm) (fixed in #10446)
  • Bug: SSR has false positive warnings for SVG tags #10415 (@spicyj) (fixed in #10452)
  • Bug: Unexpected cross-domain error passed to componentDidCatch #10441 (@bvaughn) (fixed in #10447)
  • Warn when nesting a 15 tree inside 16 (@spicyj #10434) (fixed in #10434)
  • undefined is not a function (evaluating 'owner.getName()') #10443
  • Regressions reported in beta
    • The server renderer crashes in production with inline styles. (#10299, fixed by #10300)
    • The server renderer doesn't yet support returning arrays and strings from components (fixed by #10221)
    • The server renderer still renders data-reactid somewhat unnecessarily. (#10306) (@gaearon)
    • In some cases Error: null is reported instead of the real error. (#10321) [decision: update the error message] (@acdlite)
    • Shallow renderer doesn't implement unstable_batchedUpdates. [decision: team seems to be leaning away from doing this; discussion in #10355]
  • [s-m] shouldComponentUpdate with functional components [decision: delete it for now, add it in a minor] (@sebmarkbage) #10371
  • There is a report that Google crawler can’t render the page using 16 (link). (@flarnie)
  • Check if #10405 is a real regression (No, it's been fixed looks like.)
  • [s-m] Ensure type validation didn't regress when we enabled new features #9577 (@gaearon)
  • Decide if we want to break value attribute syncing behavior (#10150) [decided to postpone until 17]
  • [s-m] Decide if we want to stop calling componentDidUpdate from shallow renderer (#10372)
  • Decide: what do we do about polyfills?
  • Big missing pieces
    • Add server rendering and reviving
    • Add shallow renderer
      • We seem to be decoupling shallow renderer from ReactDOM. However in 15 findDOMNode() works inside of shallow renderer because it contains ReactDOM injections. Is this going to be a breaking change? (@gaearon) [decision: let's not support this]
  • Decide on whether to include warning whitelisting/blacklisting (@bvaughn) [decision: not in 16.0]
  • Maybe: Switch to flat bundles (no more react-dom/lib/*, internals are truly private)
    • Decide: plan forward for popular projects depending on internals (react-native-web, react-tap-event-plugin) (might not block final)
      • EventPluginHub is "dangerously" exported from flat bundle for tap event plugin
      • react-native-web probably needs more internals, coordinate with @necolas (done in #10138)
  • Support class and for #10169 (@gaearon) [decision: not gonna do this now]
  • Feature parity for renderers
    • Shallow renderer supports strings and arrays
    • Server renderer supports strings and arrays
    • Server renderer supports error boundaries [doesn't make sense conceptually right now]
  • Decide on whether to put react-current-owner in its own package to reduce issues caused by npm duplication [not now]
  • Decide on DOMServer entry points (react-dom/server and react-dom/node-stream seem inconsistent with each other. Why is only one of them server?) (@gaearon) #10362
    • These were configured separately for Prepack. One target includes both the pure JS APIs and the Node APIs and the other only includes the pure JS APIs. We could combine them into a single react-dom/server entry point and add a new renderToString method on it.
  • Decide: Do we care about catching errors in events in the initial release? [no]
  • Error messages (@flarnie) (Dan: I moved this back to blockers because I don't think we can release with unclear messages. Error boundaries are a huge change in open source and we must have documentation and a link before we start unmounting roots.)
    • "You can add an error boundary" message should link to docs
    • duplicate key warning for children with same keys is out of date; claims child will be ignored but now we will only render second one. (easy to add to 16 final or sooner) (@sebmarkbage)
  • Verify that the bundles produced are valid in strict mode. #10294 (comment) (@bvaughn)
  • Testing the Beta: (React team in general)
    • Share the beta and proposed release date with library authors several weeks in advance
    • Triage any issues reported with beta.
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 24, 2017

Member

ReactDOM fixes, if @jquense @nhunzaker @aweary want to release any.

I think at least #8575 and #7359 should be included in 15.5. They're both close to being merged.

Member

aweary commented Jan 24, 2017

ReactDOM fixes, if @jquense @nhunzaker @aweary want to release any.

I think at least #8575 and #7359 should be included in 15.5. They're both close to being merged.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jan 24, 2017

Collaborator

@jquense we probably want to merge yours first (#8575). #7359 relies on ChangeEventPlugin to keep the value attribute in sync with the value property. We'll need to re-evaluate #7359 with those changes.

Collaborator

nhunzaker commented Jan 24, 2017

@jquense we probably want to merge yours first (#8575). #7359 relies on ChangeEventPlugin to keep the value attribute in sync with the value property. We'll need to re-evaluate #7359 with those changes.

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Jan 24, 2017

  • React.createClass will be moved to a separate package
  • React.createFactory will be moved to a separate package
  • React.DOM.* factories will be moved to a separate package

All these new packages, maybe you need a tool for managing so many packages in a single repo... 😉

  • React.createClass will be moved to a separate package
  • React.createFactory will be moved to a separate package
  • React.DOM.* factories will be moved to a separate package

All these new packages, maybe you need a tool for managing so many packages in a single repo... 😉

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jan 24, 2017

Member

As long as it doesn't make our UMD builds harder I'm on board with lerna. Someone want to send a PR?

Member

sophiebits commented Jan 24, 2017

As long as it doesn't make our UMD builds harder I'm on board with lerna. Someone want to send a PR?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

Pretty sure we've reached peak harder and it's downhill from there.

Member

gaearon commented Jan 24, 2017

Pretty sure we've reached peak harder and it's downhill from there.

@jamiebuilds

This comment has been minimized.

Show comment
Hide comment
@jamiebuilds

jamiebuilds Jan 24, 2017

@gaearon Challenge accepted.

@spicyj jk I won't make it harder, I'll throw something together for you

@gaearon Challenge accepted.

@spicyj jk I won't make it harder, I'll throw something together for you

@tizmagik

This comment has been minimized.

Show comment
Hide comment
@tizmagik

tizmagik Jan 24, 2017

This is awesome! Excited about what's ahead in React 16. Also glad I won't have to manually create a minified build of React for server render. 😅

Curious, any idea about how much larger the bundle is with React Fiber?

This is awesome! Excited about what's ahead in React 16. Also glad I won't have to manually create a minified build of React for server render. 😅

Curious, any idea about how much larger the bundle is with React Fiber?

@klzns

This comment has been minimized.

Show comment
Hide comment
@klzns

klzns Jan 24, 2017

Why is this scratched?

Make setState async by default.
Remove the attribute whitelist.
Stateful functional components.
New context API.

If it's just a stylistic choice, it's confusing. It's like a double negative hehe

klzns commented Jan 24, 2017

Why is this scratched?

Make setState async by default.
Remove the attribute whitelist.
Stateful functional components.
New context API.

If it's just a stylistic choice, it's confusing. It's like a double negative hehe

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jan 24, 2017

Collaborator

Ya! look great y'all. one thought related to flat bundles. it's harder to write certain types of tooling without access to internal apis. Specifically thinking about test libs, the few I maintain invariably need to touch a bunch of internal apis when test utils fall short. it's a bit of a niche use case tho asandthe cost of using those Apis is well understood and aren't used in production apps

Collaborator

jquense commented Jan 24, 2017

Ya! look great y'all. one thought related to flat bundles. it's harder to write certain types of tooling without access to internal apis. Specifically thinking about test libs, the few I maintain invariably need to touch a bunch of internal apis when test utils fall short. it's a bit of a niche use case tho asandthe cost of using those Apis is well understood and aren't used in production apps

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Jan 24, 2017

Contributor

If you guys do want to switch to lerna, I've been working on rollup scripting to make flat files of lerna packages. Would be happy to help out.

Contributor

probablyup commented Jan 24, 2017

If you guys do want to switch to lerna, I've been working on rollup scripting to make flat files of lerna packages. Would be happy to help out.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

Curious, any idea about how much larger the bundle is with React Fiber?

Right now it's roughly the same size while implementing more features (async scheduling, error boundaries, fragments, portals). We haven't worked on optimizing its size so I think we can reduce it further too. We'll likely do so after the initial release.

Another promising (IMO) area is Fiber's support for custom renderers. If ReactDOM's tradeoffs don't match what your product needs, it should be much easier to create a custom DOM renderer that, for example, doesn't ship with the synthetic event system. Fiber makes custom renderers way easier to create, while giving you all component features out of the box.

Member

gaearon commented Jan 24, 2017

Curious, any idea about how much larger the bundle is with React Fiber?

Right now it's roughly the same size while implementing more features (async scheduling, error boundaries, fragments, portals). We haven't worked on optimizing its size so I think we can reduce it further too. We'll likely do so after the initial release.

Another promising (IMO) area is Fiber's support for custom renderers. If ReactDOM's tradeoffs don't match what your product needs, it should be much easier to create a custom DOM renderer that, for example, doesn't ship with the synthetic event system. Fiber makes custom renderers way easier to create, while giving you all component features out of the box.

@Hurtak

This comment has been minimized.

Show comment
Hide comment
@Hurtak

Hurtak Jan 24, 2017

'Stateful functional components' is there some issue where I can read more about this?

Hurtak commented Jan 24, 2017

'Stateful functional components' is there some issue where I can read more about this?

@swernerx

This comment has been minimized.

Show comment
Hide comment
@swernerx

swernerx Jan 24, 2017

When you go the flat bundle route - which I definitely appreciate - it would be awesome to offer a modern bundle for Node v6 / ES2015 capable browsers, too.

I did some work on this topic in http://npmjs.com/package/prepublish - which effectively produces flat bundles for different output scenarios. Basically a configured wrapper around Rollup/Babel/Buble.

I also thought of adding specific "modern" entries to the package.json so that bundles like Webpack can make use of this e.g. main:modern or module:modern. I think something like this would be definitely interesting for React (especially in context of SSR apps where you can target Node >= v6).

When you go the flat bundle route - which I definitely appreciate - it would be awesome to offer a modern bundle for Node v6 / ES2015 capable browsers, too.

I did some work on this topic in http://npmjs.com/package/prepublish - which effectively produces flat bundles for different output scenarios. Basically a configured wrapper around Rollup/Babel/Buble.

I also thought of adding specific "modern" entries to the package.json so that bundles like Webpack can make use of this e.g. main:modern or module:modern. I think something like this would be definitely interesting for React (especially in context of SSR apps where you can target Node >= v6).

@Kitanotori

This comment has been minimized.

Show comment
Hide comment
@Kitanotori

Kitanotori Jan 24, 2017

Agree with @swernerx . It's about time to migrate to ES6.

Agree with @swernerx . It's about time to migrate to ES6.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

@swernerx This is an interesting direction and we may explore it in the future. Right now the browser support for ES6 is increasing but many features are sufficiently slower than ES5 counterparts, just because nobody had time to optimize it yet. I know V8 team is hard at work fixing this, and I’d rather look into it again after evergreen browsers are mostly fast when using ES6.

Member

gaearon commented Jan 24, 2017

@swernerx This is an interesting direction and we may explore it in the future. Right now the browser support for ES6 is increasing but many features are sufficiently slower than ES5 counterparts, just because nobody had time to optimize it yet. I know V8 team is hard at work fixing this, and I’d rather look into it again after evergreen browsers are mostly fast when using ES6.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

'Stateful functional components' is there some issue where I can read more about this?

No specific issue AFAIK. There are some old ideas in this repo if you’re interested, but they’re just that, ideas. There are numerous reasons we’re interested in having stateful functional components, I listed some here.

Member

gaearon commented Jan 24, 2017

'Stateful functional components' is there some issue where I can read more about this?

No specific issue AFAIK. There are some old ideas in this repo if you’re interested, but they’re just that, ideas. There are numerous reasons we’re interested in having stateful functional components, I listed some here.

@dtinth

This comment has been minimized.

Show comment
Hide comment
@dtinth

dtinth Jan 24, 2017

Concerning the “flat bundles,” it would be great if the internals are still exposed one way or the other, because we at Taskworld sometimes monkey-patched React internals from userland code to have more fine-grained error messages and performance measurement.

What we did is we hooked into React’s internals and added some instrumenting code like this:

Show code
  const ReactReconciler = require('react/lib/ReactReconciler')

  ReactReconciler.mountComponent = ((mountComponent) => function (internalInstance) {
    enter('React mount ' + describeInstance(internalInstance))
    try { return mountComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.mountComponent)

  ReactReconciler.receiveComponent = ((receiveComponent) => function (internalInstance) {
    enter('React update ' + describeInstance(internalInstance))
    try { return receiveComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.receiveComponent)

  ReactReconciler.unmountComponent = ((unmountComponent) => function (internalInstance) {
    enter('React unmount ' + describeInstance(internalInstance))
    try { return unmountComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.unmountComponent)

We also hooked into Redux and Reselect in similar ways for a unified development experience.

I know that this isn’t supported and internal APIs may break in any release (especially with Fiber coming), but unsupported is better than impossible 😄.

dtinth commented Jan 24, 2017

Concerning the “flat bundles,” it would be great if the internals are still exposed one way or the other, because we at Taskworld sometimes monkey-patched React internals from userland code to have more fine-grained error messages and performance measurement.

What we did is we hooked into React’s internals and added some instrumenting code like this:

Show code
  const ReactReconciler = require('react/lib/ReactReconciler')

  ReactReconciler.mountComponent = ((mountComponent) => function (internalInstance) {
    enter('React mount ' + describeInstance(internalInstance))
    try { return mountComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.mountComponent)

  ReactReconciler.receiveComponent = ((receiveComponent) => function (internalInstance) {
    enter('React update ' + describeInstance(internalInstance))
    try { return receiveComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.receiveComponent)

  ReactReconciler.unmountComponent = ((unmountComponent) => function (internalInstance) {
    enter('React unmount ' + describeInstance(internalInstance))
    try { return unmountComponent.apply(this, arguments) } finally { exit() }
  })(ReactReconciler.unmountComponent)

We also hooked into Redux and Reselect in similar ways for a unified development experience.

I know that this isn’t supported and internal APIs may break in any release (especially with Fiber coming), but unsupported is better than impossible 😄.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

we at Taskworld sometimes monkey-patched React internals from userland code to have more fine-grained error messages

This is supported by error boundaries in Fiber.

and performance measurement.

You can use ReactPerf for that.

I know that this isn’t supported and internal APIs may break in any release (especially with Fiber coming), but unsupported is better than impossible 😄.

Even better, let’s discuss the use cases and come up with supported APIs. 😉

Member

gaearon commented Jan 24, 2017

we at Taskworld sometimes monkey-patched React internals from userland code to have more fine-grained error messages

This is supported by error boundaries in Fiber.

and performance measurement.

You can use ReactPerf for that.

I know that this isn’t supported and internal APIs may break in any release (especially with Fiber coming), but unsupported is better than impossible 😄.

Even better, let’s discuss the use cases and come up with supported APIs. 😉

@swernerx

This comment has been minimized.

Show comment
Hide comment
@swernerx

swernerx Jan 24, 2017

@gaearon It's about offering such a package format for modern clients (in parallel to the classic build - which is what the prepublish tool mentioned does). In the end React does not decide on the processing/bundling/output of the application. But if you transpile stuff you make the original ES6 code unaccessible for any user who wants to deploy for modern node or embedded browsers only.

@gaearon It's about offering such a package format for modern clients (in parallel to the classic build - which is what the prepublish tool mentioned does). In the end React does not decide on the processing/bundling/output of the application. But if you transpile stuff you make the original ES6 code unaccessible for any user who wants to deploy for modern node or embedded browsers only.

@swernerx

This comment has been minimized.

Show comment
Hide comment
@swernerx

swernerx Jan 24, 2017

@gaearon "main" and "module" should always point to the ES5 bundles with either commonjs or esmodule outputs. But we are free to offer an alternative modern build keeping ES2015 stuff intact.

@gaearon "main" and "module" should always point to the ES5 bundles with either commonjs or esmodule outputs. But we are free to offer an alternative modern build keeping ES2015 stuff intact.

@MilosRasic

This comment has been minimized.

Show comment
Hide comment
@MilosRasic

MilosRasic Jan 24, 2017

Excuse my ignorance, but does removal of createMixin mean that all packages that rely on mixins will stop working until dependants switch to a HOC solution or will they be able to quickly switch to an alternate way to provide old-style mixins?

Excuse my ignorance, but does removal of createMixin mean that all packages that rely on mixins will stop working until dependants switch to a HOC solution or will they be able to quickly switch to an alternate way to provide old-style mixins?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

Excuse my ignorance, but does removal of createMixin mean that all packages that rely on mixins will stop working until dependants switch to a HOC solution or will they be able to quickly switch to an alternate way to provide old-style mixins?

createMixin implementation looks like this:

function createMixin(mixin) {
  return mixin;
}

It was never documented, and only appeared in the API because we thought we might want to validate mixins in the future. Packages that somehow found out about it and started using it can just remove the createMixin call and use the mixin directly.

Most packages providing mixins don’t use createMixin. For those that do, this is a one-line change: just removing the useless function call.

It's about offering such a package format for modern clients (in parallel to the classic build - which is what the prepublish tool mentioned does)

Yes, I understand this. What I’m saying is if we offer this build, people will use it, and if performance is suboptimal, they will blame React. So we will wait until ES6 features are actually fast before providing a build that does that.

Member

gaearon commented Jan 24, 2017

Excuse my ignorance, but does removal of createMixin mean that all packages that rely on mixins will stop working until dependants switch to a HOC solution or will they be able to quickly switch to an alternate way to provide old-style mixins?

createMixin implementation looks like this:

function createMixin(mixin) {
  return mixin;
}

It was never documented, and only appeared in the API because we thought we might want to validate mixins in the future. Packages that somehow found out about it and started using it can just remove the createMixin call and use the mixin directly.

Most packages providing mixins don’t use createMixin. For those that do, this is a one-line change: just removing the useless function call.

It's about offering such a package format for modern clients (in parallel to the classic build - which is what the prepublish tool mentioned does)

Yes, I understand this. What I’m saying is if we offer this build, people will use it, and if performance is suboptimal, they will blame React. So we will wait until ES6 features are actually fast before providing a build that does that.

@Kitanotori

This comment has been minimized.

Show comment
Hide comment
@Kitanotori

Kitanotori Jan 24, 2017

@gaearon Migration to ES6 can be done in iterations, e.g., first convert all vars to either let or const, then other features that are supported without performance issues.

Also, there are two issues here: (1) syntax of the source code, and (2) syntax of the bundle. Is there a reason for not migrating the source to the latest standard - ES7/ES2016 - with the possibility of using staged features if deemed useful enough?

Syntax of the bundle is a totally separate issue, as the syntax of the bundle (or bundles) can be decided during the build step. I think it would be awesome to have separate bundles for each relevant standard, but I'm not sure how to achieve this in practice, or whether this can be achieved without unreasonable amount of work. Nevertheless, ES6 is here to stay, and the project ought to provide bundle targeting it, which naturally first requires converting the source to utilize its features.

@gaearon Migration to ES6 can be done in iterations, e.g., first convert all vars to either let or const, then other features that are supported without performance issues.

Also, there are two issues here: (1) syntax of the source code, and (2) syntax of the bundle. Is there a reason for not migrating the source to the latest standard - ES7/ES2016 - with the possibility of using staged features if deemed useful enough?

Syntax of the bundle is a totally separate issue, as the syntax of the bundle (or bundles) can be decided during the build step. I think it would be awesome to have separate bundles for each relevant standard, but I'm not sure how to achieve this in practice, or whether this can be achieved without unreasonable amount of work. Nevertheless, ES6 is here to stay, and the project ought to provide bundle targeting it, which naturally first requires converting the source to utilize its features.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

We are pragmatic in choosing both source and target languages, and are driven by pragmatic needs.

Migration to ES6 can be done in iterations, e.g., first convert all vars to either let or const, then other features that are supported without performance issues.

For the target bundle, what exactly would that accomplish? There are no benefits to the end users in this case, and there are downsides (less browser support, more fragmentation and potential surface area for bugs).

Is there a reason for not migrating the source to the latest standard - ES7/ES2016 - with the possibility of using staged features if deemed useful enough?

We are using new features in newly written code. There is very little benefit, in our opinion, to rewrite existing code with new features since this also introduces a risk of new bugs. It’s a nice way to ramp up contributors, but as noted in this issue, we have already rewritten most of React so it doesn’t make sense to me to convert the old code now.

Member

gaearon commented Jan 24, 2017

We are pragmatic in choosing both source and target languages, and are driven by pragmatic needs.

Migration to ES6 can be done in iterations, e.g., first convert all vars to either let or const, then other features that are supported without performance issues.

For the target bundle, what exactly would that accomplish? There are no benefits to the end users in this case, and there are downsides (less browser support, more fragmentation and potential surface area for bugs).

Is there a reason for not migrating the source to the latest standard - ES7/ES2016 - with the possibility of using staged features if deemed useful enough?

We are using new features in newly written code. There is very little benefit, in our opinion, to rewrite existing code with new features since this also introduces a risk of new bugs. It’s a nice way to ramp up contributors, but as noted in this issue, we have already rewritten most of React so it doesn’t make sense to me to convert the old code now.

@zalmoxisus

This comment has been minimized.

Show comment
Hide comment
@zalmoxisus

zalmoxisus Jan 24, 2017

@gaearon

Even better, let’s discuss the use cases and come up with supported APIs.

It would be great to have an API to detect if an object is a Synthetic Event. For Redux, usually users are passing events as action payloads (which also redux-actions does), even when not using them. It breaks the ability to access them while sending to the monitors. A simple solution was:

import SyntheticEvent from 'react/lib/SyntheticEvent';
// ...

const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: {
    replacer: (key, value) => {
      if (value && value instanceof SyntheticEvent) return '[Event]';
      return value;
    }
  }
}));

Though maybe we should just encourage using event.persist when not providing event data explicitly.

zalmoxisus commented Jan 24, 2017

@gaearon

Even better, let’s discuss the use cases and come up with supported APIs.

It would be great to have an API to detect if an object is a Synthetic Event. For Redux, usually users are passing events as action payloads (which also redux-actions does), even when not using them. It breaks the ability to access them while sending to the monitors. A simple solution was:

import SyntheticEvent from 'react/lib/SyntheticEvent';
// ...

const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: {
    replacer: (key, value) => {
      if (value && value instanceof SyntheticEvent) return '[Event]';
      return value;
    }
  }
}));

Though maybe we should just encourage using event.persist when not providing event data explicitly.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

Though maybe we should just encourage using event.persist when not providing event data explecitely.

This is the solution here.

Member

gaearon commented Jan 24, 2017

Though maybe we should just encourage using event.persist when not providing event data explecitely.

This is the solution here.

@Kitanotori

This comment has been minimized.

Show comment
Hide comment
@Kitanotori

Kitanotori Jan 24, 2017

@gaearon I totally understand if code that may soon be deprecated is not rewritten, but is it really a good idea to, for example, continue using two module syntaxes side-by-side in newly written code? I see some source files even combining both. For parts that are necessary for supporting Node, using Node's module syntax is of course understandable. Personally I feel quite strongly about unnecessarily mixing two competing module handling mechanisms - particularly in newly written code, and particularly when talking about a standard- and a non-standard one.

One point, not sure if actually relevant, came to my mind: React is one of the "hottest" libraries out there in the JavaScript world, and if React decided to fully utilize the latest standardized ES features (preferably with strict style guidelines), this might incentivize browser manufacturers to hasten their optimization efforts. React could show way for, not only other UI libraries out there, but for the whole JavaScript ecosystem. Maybe slightly optimistic, but 😺

@gaearon I totally understand if code that may soon be deprecated is not rewritten, but is it really a good idea to, for example, continue using two module syntaxes side-by-side in newly written code? I see some source files even combining both. For parts that are necessary for supporting Node, using Node's module syntax is of course understandable. Personally I feel quite strongly about unnecessarily mixing two competing module handling mechanisms - particularly in newly written code, and particularly when talking about a standard- and a non-standard one.

One point, not sure if actually relevant, came to my mind: React is one of the "hottest" libraries out there in the JavaScript world, and if React decided to fully utilize the latest standardized ES features (preferably with strict style guidelines), this might incentivize browser manufacturers to hasten their optimization efforts. React could show way for, not only other UI libraries out there, but for the whole JavaScript ecosystem. Maybe slightly optimistic, but 😺

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

using two module syntaxes side-by-side in newly written code? I see some source files even combining both.

We are using CommonJS exclusively right now (mostly due to Facebook build pipeline limitations). I think you might be confusing Flow's import type syntax with ES modules.

Member

gaearon commented Jan 24, 2017

using two module syntaxes side-by-side in newly written code? I see some source files even combining both.

We are using CommonJS exclusively right now (mostly due to Facebook build pipeline limitations). I think you might be confusing Flow's import type syntax with ES modules.

@Kitanotori

This comment has been minimized.

Show comment
Hide comment
@Kitanotori

Kitanotori Jan 24, 2017

Oh, haven't used Flow so that kind of syntax was new for me.

Oh, haven't used Flow so that kind of syntax was new for me.

@dtinth

This comment has been minimized.

Show comment
Hide comment
@dtinth

dtinth Jan 24, 2017

@gaearon Thanks for the suggestions. I am excited about the error boundaries!

I find off-the-shelf solutions (such as ReactPerf) sometimes not to be suitable. For instance, I monkeypatched several libraries to be able to construct a table that shows how much time (in percentage) is spent in:

  • React reconciling and doing DOM manipulations (react-dom/lib/ReactReconciler)
  • Userland React lifecycle (react-dom/lib/ReactInstrumentation)
  • Selectors created using reselect
  • The Redux store reducer
  • The Redux store subscriber

Having this data gives a high level overview of where I should focus on optimizing.

However, let’s not focus on a specific problem (performance, error boundaries)—these are just examples. I believe there are other unforeseen use cases where monkeypatching the internals could be useful.

Here’s another situation where I monkeypatched React’s internals: I want to make React warnings appear as a popup in the DOM instead of in the console, because sometimes I develop my user interface without the devtools on.

I accomplished this by monkeypatching fbjs/lib/warning to display a popup instead of using console.error(). This would be very hard to accomplish if React is packaged as a flat bundle—I would have given up that idea. Exposing the internals allows for experimentation.

dtinth commented Jan 24, 2017

@gaearon Thanks for the suggestions. I am excited about the error boundaries!

I find off-the-shelf solutions (such as ReactPerf) sometimes not to be suitable. For instance, I monkeypatched several libraries to be able to construct a table that shows how much time (in percentage) is spent in:

  • React reconciling and doing DOM manipulations (react-dom/lib/ReactReconciler)
  • Userland React lifecycle (react-dom/lib/ReactInstrumentation)
  • Selectors created using reselect
  • The Redux store reducer
  • The Redux store subscriber

Having this data gives a high level overview of where I should focus on optimizing.

However, let’s not focus on a specific problem (performance, error boundaries)—these are just examples. I believe there are other unforeseen use cases where monkeypatching the internals could be useful.

Here’s another situation where I monkeypatched React’s internals: I want to make React warnings appear as a popup in the DOM instead of in the console, because sometimes I develop my user interface without the devtools on.

I accomplished this by monkeypatching fbjs/lib/warning to display a popup instead of using console.error(). This would be very hard to accomplish if React is packaged as a flat bundle—I would have given up that idea. Exposing the internals allows for experimentation.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

I find off-the-shelf solutions (such as ReactPerf) sometimes not to be suitable. For instance, I monkeypatched several libraries to be able to construct a table that shows how much time (in percentage) is spent in: [...]

I may be wrong but I believe all this data is already exposed via ReactPerf.getLastMeasurements().

I want to make React warnings appear as a popup in the DOM instead of in the console, because sometimes I develop my user interface without the devtools on.

So do we which is why we intend to show a UI (#7360) by default in React 16. I forgot to add this to the umbrella.

let’s not focus on a specific problem (performance, error boundaries)—these are just examples. I believe there are other unforeseen use cases where monkeypatching the internals could be useful.

I agree it can be useful. However the problem is, once you've monkeypatched it to your convenience, you won't come back filing an issue and gathering support for it from the community. As a result everyone ends up with local hacks that break every few versions, and no permanent solutions, while the maintainers aren't even aware this is something people want. I would rather prefer we have more discussions like this one, where you explain the use cases, and we either point out or provide (possibly unstable) hooks.

Member

gaearon commented Jan 24, 2017

I find off-the-shelf solutions (such as ReactPerf) sometimes not to be suitable. For instance, I monkeypatched several libraries to be able to construct a table that shows how much time (in percentage) is spent in: [...]

I may be wrong but I believe all this data is already exposed via ReactPerf.getLastMeasurements().

I want to make React warnings appear as a popup in the DOM instead of in the console, because sometimes I develop my user interface without the devtools on.

So do we which is why we intend to show a UI (#7360) by default in React 16. I forgot to add this to the umbrella.

let’s not focus on a specific problem (performance, error boundaries)—these are just examples. I believe there are other unforeseen use cases where monkeypatching the internals could be useful.

I agree it can be useful. However the problem is, once you've monkeypatched it to your convenience, you won't come back filing an issue and gathering support for it from the community. As a result everyone ends up with local hacks that break every few versions, and no permanent solutions, while the maintainers aren't even aware this is something people want. I would rather prefer we have more discussions like this one, where you explain the use cases, and we either point out or provide (possibly unstable) hooks.

@dtinth

This comment has been minimized.

Show comment
Hide comment
@dtinth

dtinth Jan 24, 2017

@gaearon That makes sense, and I agree with your sentiment. Although I usually treat open-source projects with the “you want it, you do it” mindset (which is why I kinda like to start by experimenting alone), making them an issue could bring more benefit to the project and community. ❤️

dtinth commented Jan 24, 2017

@gaearon That makes sense, and I agree with your sentiment. Although I usually treat open-source projects with the “you want it, you do it” mindset (which is why I kinda like to start by experimenting alone), making them an issue could bring more benefit to the project and community. ❤️

@MHerszak

This comment has been minimized.

Show comment
Hide comment
@MHerszak

MHerszak Jan 24, 2017

I can see my browser console showing so many warnings just because of react-router. :) And also other packages that I use which still make use of createClass.

MHerszak commented Jan 24, 2017

I can see my browser console showing so many warnings just because of react-router. :) And also other packages that I use which still make use of createClass.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

We likely won’t be firing multiple warnings, just a single one (with names of the first few classes). We don’t want to spam the console.

Member

gaearon commented Jan 24, 2017

We likely won’t be firing multiple warnings, just a single one (with names of the first few classes). We don’t want to spam the console.

@guotie

This comment has been minimized.

Show comment
Hide comment
@guotie

guotie Jan 25, 2017

when does it release

guotie commented Jan 25, 2017

when does it release

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 25, 2017

Member

We don't know yet but I'd say likely within three months.

Member

gaearon commented Jan 25, 2017

We don't know yet but I'd say likely within three months.

@lax4mike

This comment has been minimized.

Show comment
Hide comment
@lax4mike

lax4mike Jan 25, 2017

+1 for Stateful functional components. I don't know what the API would look like, but it has to be more elegant than es6 classes.

+1 for Stateful functional components. I don't know what the API would look like, but it has to be more elegant than es6 classes.

@voidale

This comment has been minimized.

Show comment
Hide comment
@voidale

voidale Jan 25, 2017

that's really awesome cant wait to get Fiber means we can finally improve the renderToString to achieve better SSR performance? Async renderToString will be the best thing for me right now went from 2 servers to 10 just because it's not async.

voidale commented Jan 25, 2017

that's really awesome cant wait to get Fiber means we can finally improve the renderToString to achieve better SSR performance? Async renderToString will be the best thing for me right now went from 2 servers to 10 just because it's not async.

flarnie added a commit to flarnie/react that referenced this issue Jul 19, 2017

Force an 'any' type for the `hostInstance.parentNode` in warning check
**what is the change?:**
This is awful. :(
I'm not sure how else to let Flow know that we expect that this might be
a sort of `DOMContainer` type and not just a normal `Node` type.

To at least make the type information clear we added a comment.

**why make this change?:**
To get `flow` passing. Looks like we have `any` types sprinkled
throughout this file. phooey. :(

**test plan:**
`yarn flow`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Ignore portals in `DOMRenderer.findHostInstance`
**what is the change?:**
We want to ignore portals when firing a certain warning.

This allows us to get the host instance and ignore portals.

Also added a new snapshot recording while fixing things.

**why make this change?:**
Originally we had added a flag to the DOM node which was used for

rendering the portal, and then could notice and ignore children rendered
into those nodes.

However, it's better to just ignore portals in
`DOMRenderer.findHostInstance` because
 - we will not ignore a non-portal second child with this approach
 - we meant to ignore portals in this method anyway (according to a
   'TODO' comment)
 - this change only affects the DOM renderer, instead of changing code
   which is shared with RN and other renderers
 - we avoid adding unneeded expandos

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

RFC Add warning for rendering into container that was updated manually
RFC because we still need to tidy this up and verify that all tests
pass.

**what is the change?:**
We want to warn when users render into a container which was manually
emptied or updated outside of React. This can lead to the cryptic error
about not being able to remove a node, or just lead to silent failures
of render. This warning should make things more clear.

Note that this covers the case where the contents of the root container
are manually updated, but does not cover the case where something was
manually updated deeper in the tree.

**why make this change?:**
To maintain parity and increase clarity before releasing v16.0 beta.

**test plan:**
`yarn test`

**issue:**
facebook#8854

last item under the '16 beta' checklist.

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Add test and tweak check for rendering into manually updated container
STILL TODO: figure out how to skip this warning when the component
renders to a portal.

Unfortunately 'ReactPortal.isPortal(children)' returns false, even in
the failing test where we are rendering to a portal.

**what is the change?:**
- added a test for the case where we call 'ReactDOM.render' with a new
  container, using a key or a different type, after the contents of the
  first container were messed with outside of React. This case throws,
  and now at least there will be an informative warning along with the
  error.
- We updated the check to compare the parent of the 'hostInstance' to
  the container; this seems less fragile
- tweaked some comments

**why make this change?:**
Continue improving this to make it more final.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Stub our `console.error` in one of the portal tests
**what is the change?:**
See title

**why make this change?:**
See comment in the code

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Skip warning in 'ReactDOMFiberEntry' when mounting to Comment node
**what is the change?:**
We have a warning for cases when the container doesn't match the parent
which we remembered the previously rendered content being rendered into.

We are skipping that warning when you render into a 'comment' node.

**why make this change?:**
Basically, if you render into a 'comment' node, then the parent of the
comment node is the container for your rendered content. We could check
for similarity there but rendering into a comment node seems like a
corner case and I'd rather skip the warning without knowing more about
what could happen in that case.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Improve warning message, remove dedup check, and unmock console.error
**what is the change?:**
Various changes to get this closer to being finished;
- Improved warning message (thanks @spicyj!!!)
- Removed dedup check on warning
- Remove mocking of 'console.error' in portals test

**why make this change?:**
- warning message improvement: communicates better with users
- Remove dedup check: it wasn't important in this case
- Remove mocking of 'console.error'; we don't want to ignore an
  inaccurate warning, even for an "unstable" feature.

**test plan:**
`yarn test` -> follow-up commits will fix the remaining tests

**issue:**
issue #8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Possible fix for issue of incorrect warning for portal re-render
**what is the change?:**
Add a property to a container which was rendered into using
`ReactDOM.unstable_createPortal`.

**why make this change?:**
We don't want to warn for mismatching container nodes in this case - the
user intentionally rendered into the portal container instead of the
original container.

concerns;
- will this affect React Native badly?
- will this add bloat to the portal code? seems small enough but not
  sure.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Fix logic for checking if the host instance container is a portal
**what is the change?:**
When focusing on fixing the warning to not check when we are using
portals, I missed checking for the existence of the host instance parent
before checking if it was a portal. This adds the missing null checks.

**why make this change?:**
To fix a bug that the previous commit introduced.

**test plan:**
`yarn test`
-> follow-up commits fix more of the test failures

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Clean up new tests in ReactDOMFiber-test
**what is the change?:**
- removed extra single quotes, downgrade double quotes to single
- update expected warning message to match latest warning message
- fix indentation

**why make this change?:**
- get tests passing
- code maintainability/readability

**test plan:**
`yarn test`
follow up commits will fix the remaining tests

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Add 'unmountComponentAtNode' call in test for reconciling pre-rendere…
…d markup

**what is the change?:**
We have a test that verifies React can reconcile text from pre-rendered
mark-up. It tests React doing this for three strings and three empty
strings.

This adds a call to 'unmountComponentAtNode' between the two
expectations for strings and empty strings.

**why make this change?:**
We now warn when someone messes with the DOM inside of a node in such a
way that removes the React-rendered content. This test was doing that. I
can't think of a situation where this would happen with server-side
rendering without the need to call 'unmountComponentAtNode' before
inserting the server-side rendered content.

**test plan:**
`yarn test`

Only one more failing test, will fix that in the next commit.

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Fix type error and improve name of portal container flag
**NOTE:** I am still looking for a good place to move this flag
assignment to, or a better approach. This does some intermediate fixes.

**what is the change?:**
- fixed flow error by allowing optional flag on a DOMContainer that
  indicates it was used as a portal container.
- renamed the flag to something which makes more sense

**why make this change?:**
- get Flow passing
- make this change make more sense

We are still not sure about adding this flag; a follow-up diff may move
it or take a different approach.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Add flag to portalContainer on mount instead of in `createPortal`
**what is the change?:**
We add a flag to the container of a 'portal' in the 'commit work' phase
in Fiber. This is right before we call `appendChildToContainer`.

**why make this change?:**
- Sometimes people call `ReactDOM.render(... container)`, then manually
clear the content of the `container`, and then try to call another
`ReactDOM.render(... container)`.
- This leads to cryptic errors or silent failure because we hold a
  reference to the node that was rendered the first time, and expect it
  to still be inside the container.
- We added a warning for this issue in `renderSubtreeIntoContainer`, but
  when a component renders something returned by
  `ReactDOM.unstable_createPortal(<Component />, portalContainer);`,
  then the child is inside the `portalContainer` and not the `container,
  but that is valid and we want to skip warning in that case.

Inside `renderSubtreeIntoContainer` we don't have the info to determine
if a child was rendered into a `portalContainer` or a `container`, and
adding this flag lets us figure that out and skip the warning.

We originally added the flag in the call to
`ReactDOM.unstable_createPortal` but that seemed like a method that
should be "pure" and free of side-effects. This commit moves the
flag-adding to happen when we mount the portal component.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Force an 'any' type for the `hostInstance.parentNode` in warning check
**what is the change?:**
This is awful. :(
I'm not sure how else to let Flow know that we expect that this might be
a sort of `DOMContainer` type and not just a normal `Node` type.

To at least make the type information clear we added a comment.

**why make this change?:**
To get `flow` passing. Looks like we have `any` types sprinkled
throughout this file. phooey. :(

**test plan:**
`yarn flow`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Ignore portals in `DOMRenderer.findHostInstance`
**what is the change?:**
We want to ignore portals when firing a certain warning.

This allows us to get the host instance and ignore portals.

Also added a new snapshot recording while fixing things.

**why make this change?:**
Originally we had added a flag to the DOM node which was used for

rendering the portal, and then could notice and ignore children rendered
into those nodes.

However, it's better to just ignore portals in
`DOMRenderer.findHostInstance` because
 - we will not ignore a non-portal second child with this approach
 - we meant to ignore portals in this method anyway (according to a
   'TODO' comment)
 - this change only affects the DOM renderer, instead of changing code
   which is shared with RN and other renderers
 - we avoid adding unneeded expandos

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Remove expando that we were adding to portal container
**what is the change?:**
see title

**why make this change?:**
this is part of an old approach to detecting portals, and we have
instead added a check in the `findHostInstance` method to filter out
portals.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Jul 20, 2017

Fork `findHostInstance` to make `findHostInstanceWithNoPortals`
**what is the change?:**
We need to get host instances, but filter out portals. There is not
currently a method for that.

**why make this change?:**
Rather than change the existing `findHostInstance` method, which would
affect the behavior of the public `findDOMNode` method, we are forking.

**test plan:**
`yarn test`

**issue:**
facebook#8854

flarnie added a commit that referenced this issue Jul 21, 2017

Add warning for rendering into container that was updated manually (#…
…10210)

* RFC Add warning for rendering into container that was updated manually

RFC because we still need to tidy this up and verify that all tests
pass.

**what is the change?:**
We want to warn when users render into a container which was manually
emptied or updated outside of React. This can lead to the cryptic error
about not being able to remove a node, or just lead to silent failures
of render. This warning should make things more clear.

Note that this covers the case where the contents of the root container
are manually updated, but does not cover the case where something was
manually updated deeper in the tree.

**why make this change?:**
To maintain parity and increase clarity before releasing v16.0 beta.

**test plan:**
`yarn test`

**issue:**
#8854

last item under the '16 beta' checklist.

* Add test and tweak check for rendering into manually updated container

STILL TODO: figure out how to skip this warning when the component
renders to a portal.

Unfortunately 'ReactPortal.isPortal(children)' returns false, even in
the failing test where we are rendering to a portal.

**what is the change?:**
- added a test for the case where we call 'ReactDOM.render' with a new
  container, using a key or a different type, after the contents of the
  first container were messed with outside of React. This case throws,
  and now at least there will be an informative warning along with the
  error.
- We updated the check to compare the parent of the 'hostInstance' to
  the container; this seems less fragile
- tweaked some comments

**why make this change?:**
Continue improving this to make it more final.

**test plan:**
`yarn test`

**issue:**
#8854

* Stub our `console.error` in one of the portal tests

**what is the change?:**
See title

**why make this change?:**
See comment in the code

**test plan:**
`yarn test`

**issue:**
#8854

* Skip warning in 'ReactDOMFiberEntry' when mounting to Comment node

**what is the change?:**
We have a warning for cases when the container doesn't match the parent
which we remembered the previously rendered content being rendered into.

We are skipping that warning when you render into a 'comment' node.

**why make this change?:**
Basically, if you render into a 'comment' node, then the parent of the
comment node is the container for your rendered content. We could check
for similarity there but rendering into a comment node seems like a
corner case and I'd rather skip the warning without knowing more about
what could happen in that case.

**test plan:**
`yarn test`

**issue:**
#8854

* Improve warning message, remove dedup check, and unmock console.error

**what is the change?:**
Various changes to get this closer to being finished;
- Improved warning message (thanks @spicyj!!!)
- Removed dedup check on warning
- Remove mocking of 'console.error' in portals test

**why make this change?:**
- warning message improvement: communicates better with users
- Remove dedup check: it wasn't important in this case
- Remove mocking of 'console.error'; we don't want to ignore an
  inaccurate warning, even for an "unstable" feature.

**test plan:**
`yarn test` -> follow-up commits will fix the remaining tests

**issue:**
issue #8854

* Possible fix for issue of incorrect warning for portal re-render

**what is the change?:**
Add a property to a container which was rendered into using
`ReactDOM.unstable_createPortal`.

**why make this change?:**
We don't want to warn for mismatching container nodes in this case - the
user intentionally rendered into the portal container instead of the
original container.

concerns;
- will this affect React Native badly?
- will this add bloat to the portal code? seems small enough but not
  sure.

**test plan:**
`yarn test`

**issue:**
#8854

* Fix logic for checking if the host instance container is a portal

**what is the change?:**
When focusing on fixing the warning to not check when we are using
portals, I missed checking for the existence of the host instance parent
before checking if it was a portal. This adds the missing null checks.

**why make this change?:**
To fix a bug that the previous commit introduced.

**test plan:**
`yarn test`
-> follow-up commits fix more of the test failures

**issue:**
#8854

* Clean up new tests in ReactDOMFiber-test

**what is the change?:**
- removed extra single quotes, downgrade double quotes to single
- update expected warning message to match latest warning message
- fix indentation

**why make this change?:**
- get tests passing
- code maintainability/readability

**test plan:**
`yarn test`
follow up commits will fix the remaining tests

**issue:**
#8854

* Add 'unmountComponentAtNode' call in test for reconciling pre-rendered markup

**what is the change?:**
We have a test that verifies React can reconcile text from pre-rendered
mark-up. It tests React doing this for three strings and three empty
strings.

This adds a call to 'unmountComponentAtNode' between the two
expectations for strings and empty strings.

**why make this change?:**
We now warn when someone messes with the DOM inside of a node in such a
way that removes the React-rendered content. This test was doing that. I
can't think of a situation where this would happen with server-side
rendering without the need to call 'unmountComponentAtNode' before
inserting the server-side rendered content.

**test plan:**
`yarn test`

Only one more failing test, will fix that in the next commit.

**issue:**
#8854

* ran prettier

* remove unused variable

* run scripts/fiber/record-tests

* Fix type error and improve name of portal container flag

**NOTE:** I am still looking for a good place to move this flag
assignment to, or a better approach. This does some intermediate fixes.

**what is the change?:**
- fixed flow error by allowing optional flag on a DOMContainer that
  indicates it was used as a portal container.
- renamed the flag to something which makes more sense

**why make this change?:**
- get Flow passing
- make this change make more sense

We are still not sure about adding this flag; a follow-up diff may move
it or take a different approach.

**test plan:**
`yarn test`

**issue:**
#8854

* Add flag to portalContainer on mount instead of in `createPortal`

**what is the change?:**
We add a flag to the container of a 'portal' in the 'commit work' phase
in Fiber. This is right before we call `appendChildToContainer`.

**why make this change?:**
- Sometimes people call `ReactDOM.render(... container)`, then manually
clear the content of the `container`, and then try to call another
`ReactDOM.render(... container)`.
- This leads to cryptic errors or silent failure because we hold a
  reference to the node that was rendered the first time, and expect it
  to still be inside the container.
- We added a warning for this issue in `renderSubtreeIntoContainer`, but
  when a component renders something returned by
  `ReactDOM.unstable_createPortal(<Component />, portalContainer);`,
  then the child is inside the `portalContainer` and not the `container,
  but that is valid and we want to skip warning in that case.

Inside `renderSubtreeIntoContainer` we don't have the info to determine
if a child was rendered into a `portalContainer` or a `container`, and
adding this flag lets us figure that out and skip the warning.

We originally added the flag in the call to
`ReactDOM.unstable_createPortal` but that seemed like a method that
should be "pure" and free of side-effects. This commit moves the
flag-adding to happen when we mount the portal component.

**test plan:**
`yarn test`

**issue:**
#8854

* Force an 'any' type for the `hostInstance.parentNode` in warning check

**what is the change?:**
This is awful. :(
I'm not sure how else to let Flow know that we expect that this might be
a sort of `DOMContainer` type and not just a normal `Node` type.

To at least make the type information clear we added a comment.

**why make this change?:**
To get `flow` passing. Looks like we have `any` types sprinkled
throughout this file. phooey. :(

**test plan:**
`yarn flow`

**issue:**
#8854

* Ignore portals in `DOMRenderer.findHostInstance`

**what is the change?:**
We want to ignore portals when firing a certain warning.

This allows us to get the host instance and ignore portals.

Also added a new snapshot recording while fixing things.

**why make this change?:**
Originally we had added a flag to the DOM node which was used for

rendering the portal, and then could notice and ignore children rendered
into those nodes.

However, it's better to just ignore portals in
`DOMRenderer.findHostInstance` because
 - we will not ignore a non-portal second child with this approach
 - we meant to ignore portals in this method anyway (according to a
   'TODO' comment)
 - this change only affects the DOM renderer, instead of changing code
   which is shared with RN and other renderers
 - we avoid adding unneeded expandos

**test plan:**
`yarn test`

**issue:**
#8854

* Ran prettier

* Remove error snapshot test

I think there is a bug where an empty snapshot is treated as an 'outdated snapshot'.

If I delete the obsolute snapshot, and run ReactDOMFiber-test.js it generates a new snapshot.
But then when I run the test with the newly generated snapshot, it says "1 obsolete snapshot found",
At some point I will file an issue with Jest. For now going to skip the snapshot generation for the error message in the new test.

* Remove expando that we were adding to portal container

**what is the change?:**
see title

**why make this change?:**
this is part of an old approach to detecting portals, and we have
instead added a check in the `findHostInstance` method to filter out
portals.

**test plan:**
`yarn test`

**issue:**
#8854

* Fork `findHostInstance` to make `findHostInstanceWithNoPortals`

**what is the change?:**
We need to get host instances, but filter out portals. There is not
currently a method for that.

**why make this change?:**
Rather than change the existing `findHostInstance` method, which would
affect the behavior of the public `findDOMNode` method, we are forking.

**test plan:**
`yarn test`

**issue:**
#8854
@iddan

This comment has been minimized.

Show comment
Hide comment
@iddan

iddan Jul 25, 2017

When will I be able to return strings in component's render? As we use the 16 alpha w/ RN I thought this is already possible

iddan commented Jul 25, 2017

When will I be able to return strings in component's render? As we use the 16 alpha w/ RN I thought this is already possible

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 25, 2017

Member

RN does not use Fiber yet. The "react" package is not very relevant here because it doesn't contain the reconciler logic. It only has helpers like createElement and Component. So it's not Fiber. We'll switch RN to Fiber some time soon but don't have an exact date.

Member

gaearon commented Jul 25, 2017

RN does not use Fiber yet. The "react" package is not very relevant here because it doesn't contain the reconciler logic. It only has helpers like createElement and Component. So it's not Fiber. We'll switch RN to Fiber some time soon but don't have an exact date.

@mnpenner mnpenner referenced this issue Aug 4, 2017

Closed

Roadmap #10

7 of 13 tasks complete

@bvaughn bvaughn changed the title from React 15.5 and 16 Umbrella ☂️ to React 16 Umbrella ☂️ (and 15.5) Aug 14, 2017

flarnie added a commit to flarnie/react that referenced this issue Sep 6, 2017

Update results.json
**what is the change?:**
Ran the build after updating Yarn lockfile and recorded latest build
size results.

**why make this change?:**
Let's get an idea of bundle size at this point, when we are preparing
the 16.0 RC.

**test plan:**
NA

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Sep 6, 2017

Update error codes
**what is the change?:**
This just updated the error codes on the docs.

For my future self, recording the steps it took to get here:
- On master, run `npm run build -- --extract-errors`
- Create a commit with those changes, and open a PR
  (facebook#10619)
- Cherry-pick that commit onto 15-stable, and open a PR
  (facebook#10621)
- Update the error codes on the `gh-pages branch` by doing the
  following based on docs
  (https://github.com/facebook/react/tree/master/docs#updating-facebookgithubioreact):
  - Set up a sister directory to `react` with `gh-pages` checked out,
    and call it `react-gh-pages`
  - In your `react` repo, with `15-stable` checked out and the
    error-codes update cherry-picked, run:
    `cd docs && bundle rake exec && bundle exec rake fetch_remotes && bundle exec rake release`
  - `cd ../react-gh-pages` and check in those changes, create a
    commit/PR
  - Manually test running the docs

**why make this change?:**
Even though this is just an RC, and not a final major release, we still
want the docs to be as up-to-date as possible.

**test plan:**
Run the docs locally and make sure things work.
(Flarnie will insert screenshots.)

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Sep 6, 2017

Update results.json
**what is the change?:**
Ran the build after updating Yarn lockfile and recorded latest build
size results.

**why make this change?:**
Let's get an idea of bundle size at this point, when we are preparing
the 16.0 RC.

**test plan:**
NA

**issue:**
facebook#8854

flarnie added a commit that referenced this issue Sep 6, 2017

Update error codes and results.json (#10619)
* Update results.json

**what is the change?:**
Ran the build after updating Yarn lockfile and recorded latest build
size results.

**why make this change?:**
Let's get an idea of bundle size at this point, when we are preparing
the 16.0 RC.

**test plan:**
NA

**issue:**
#8854

* Update error codes

https://github.com/facebook/react/tree/master/scripts/release-manager#update-the-error-codes

flarnie added a commit that referenced this issue Sep 6, 2017

Update error codes (#10622)
**what is the change?:**
This just updated the error codes on the docs.

For my future self, recording the steps it took to get here:
- On master, run `npm run build -- --extract-errors`
- Create a commit with those changes, and open a PR
  (#10619)
- Cherry-pick that commit onto 15-stable, and open a PR
  (#10621)
- Update the error codes on the `gh-pages branch` by doing the
  following based on docs
  (https://github.com/facebook/react/tree/master/docs#updating-facebookgithubioreact):
  - Set up a sister directory to `react` with `gh-pages` checked out,
    and call it `react-gh-pages`
  - In your `react` repo, with `15-stable` checked out and the
    error-codes update cherry-picked, run:
    `cd docs && bundle rake exec && bundle exec rake fetch_remotes && bundle exec rake release`
  - `cd ../react-gh-pages` and check in those changes, create a
    commit/PR
  - Manually test running the docs

**why make this change?:**
Even though this is just an RC, and not a final major release, we still
want the docs to be as up-to-date as possible.

**test plan:**
Run the docs locally and make sure things work.
(Flarnie will insert screenshots.)

**issue:**
#8854

flarnie added a commit to flarnie/react that referenced this issue Sep 8, 2017

Update error codes and results.json (#10619)
* Update results.json

**what is the change?:**
Ran the build after updating Yarn lockfile and recorded latest build
size results.

**why make this change?:**
Let's get an idea of bundle size at this point, when we are preparing
the 16.0 RC.

**test plan:**
NA

**issue:**
facebook#8854

* Update error codes

https://github.com/facebook/react/tree/master/scripts/release-manager#update-the-error-codes

flarnie added a commit to flarnie/react that referenced this issue Sep 18, 2017

First shot at updating changelog for 16.0
**what is the change?:**
Added an 'unreleased' section to the changelog with info from facebook#10294

**why make this change?:**
To get things set for the 16.0 release.

**test plan:**
Visual inspection

**issue:**
facebook#8854

flarnie added a commit to flarnie/react that referenced this issue Sep 20, 2017

Add requestAnimationFrame and remove "New helpful warnings"
**what is the change?:**
In response to helpful code review -
- Add mention of dependency on `requestAnimationFrame` and need to
  polyfill that as well as `Map` and `Set`
- Remove "New helpful warnings" section; it was incomplete, and there
  are so many new and updated warnings that it might not be reasonable
  to cover them in the changelog.

**why make this change?:**
Accuracy

**test plan:**
Visual inspection

**issue:**
issue #8854

flarnie added a commit to flarnie/react that referenced this issue Sep 22, 2017

Misc. formatting/wording fixes to changelog
**what is the change?:**
Thanks to the kind code review comments of @gaearon and @nhunzaker we
have
- removed the non-deterministic bold styling on some bullet points
- improved wording of the bullet points for portals, attribute whitelist
  changes, and server rendering changes
- Add note about error boundaries including a breaking change to error
  handling behavior.
- punctuation and capitalization fixes

**why make this change?:**
Clarity and correctness

**test plan:**
Visual inspection

**issue:**
facebook#8854

flarnie added a commit that referenced this issue Sep 22, 2017

Update changelog for unreleased 16.0 changes (#10730)
* First shot at updating changelog for 16.0

**what is the change?:**
Added an 'unreleased' section to the changelog with info from #10294

**why make this change?:**
To get things set for the 16.0 release.

**test plan:**
Visual inspection

**issue:**
#8854

* Fix typos and formatting errors in changelog

* Add requestAnimationFrame and remove "New helpful warnings"

**what is the change?:**
In response to helpful code review -
- Add mention of dependency on `requestAnimationFrame` and need to
  polyfill that as well as `Map` and `Set`
- Remove "New helpful warnings" section; it was incomplete, and there
  are so many new and updated warnings that it might not be reasonable
  to cover them in the changelog.

**why make this change?:**
Accuracy

**test plan:**
Visual inspection

**issue:**
issue #8854

* Improve wording

* Improve wording and fix missing links

* Add backticks to file names & code; wording tweak

* Break "Major Changes" into "New Feature" and "Breaking Changes"

* Add server side render changes to 16.0 changelog

* Change gist link from mine to gaearons

* Add note about returning fragments

* fix misc nits

* Misc. formatting/wording fixes to changelog

**what is the change?:**
Thanks to the kind code review comments of @gaearon and @nhunzaker we
have
- removed the non-deterministic bold styling on some bullet points
- improved wording of the bullet points for portals, attribute whitelist
  changes, and server rendering changes
- Add note about error boundaries including a breaking change to error
  handling behavior.
- punctuation and capitalization fixes

**why make this change?:**
Clarity and correctness

**test plan:**
Visual inspection

**issue:**
#8854

* fix broken link

bvaughn added a commit that referenced this issue Sep 27, 2017

Merge changes from master into Gatsby branch (#10853)
* Update changelog for unreleased 16.0 changes (#10730)

* First shot at updating changelog for 16.0

**what is the change?:**
Added an 'unreleased' section to the changelog with info from #10294

**why make this change?:**
To get things set for the 16.0 release.

**test plan:**
Visual inspection

**issue:**
#8854

* Fix typos and formatting errors in changelog

* Add requestAnimationFrame and remove "New helpful warnings"

**what is the change?:**
In response to helpful code review -
- Add mention of dependency on `requestAnimationFrame` and need to
  polyfill that as well as `Map` and `Set`
- Remove "New helpful warnings" section; it was incomplete, and there
  are so many new and updated warnings that it might not be reasonable
  to cover them in the changelog.

**why make this change?:**
Accuracy

**test plan:**
Visual inspection

**issue:**
issue #8854

* Improve wording

* Improve wording and fix missing links

* Add backticks to file names & code; wording tweak

* Break "Major Changes" into "New Feature" and "Breaking Changes"

* Add server side render changes to 16.0 changelog

* Change gist link from mine to gaearons

* Add note about returning fragments

* fix misc nits

* Misc. formatting/wording fixes to changelog

**what is the change?:**
Thanks to the kind code review comments of @gaearon and @nhunzaker we
have
- removed the non-deterministic bold styling on some bullet points
- improved wording of the bullet points for portals, attribute whitelist
  changes, and server rendering changes
- Add note about error boundaries including a breaking change to error
  handling behavior.
- punctuation and capitalization fixes

**why make this change?:**
Clarity and correctness

**test plan:**
Visual inspection

**issue:**
#8854

* fix broken link

* Fixes #9667: Updated createTextInstance to create the text node on correct document (#10723)

* Record sizes

*  Add a changelog for elements having the same key (#10811)

*  Add a changelog for elements having the same key

* Reword

* Markdown fixs on "DOM Attributes in React 16" post (#10816)

* Include tag name into the table snapshot (#10818)

* Update DOM warning wording and link (#10819)

* Update DOM warning wording and link

* Consistently use "Invalid" for known misspellings

* Update license headers BSD+Patents -> MIT

Did find and replace in TextMate.

```
find: (?:( \*)( ))?Copyright (?:\(c\) )?(\d{4})\b.+Facebook[\s\S]+(?:this source tree|the same directory)\.$
replace: $1$2Copyright (c) $3-present, Facebook, Inc.\n$1\n$1$2This source code is licensed under the MIT license found in the\n$1$2LICENSE file in the root directory of this source tree.
```

* Change license and remove references to PATENTS

Only remaining references:

```
docs/_posts/2014-10-28-react-v0.12.md
51:You can read the full text of the [LICENSE](https://github.com/facebook/react/blob/master/LICENSE) and [`PATENTS`](https://github.com/facebook/react/blob/master/PATENTS) files on GitHub.

docs/_posts/2015-04-17-react-native-v0.4.md
20:* **Patent Grant**: Many of you had concerns and questions around the PATENTS file. We pushed [a new version of the grant](https://code.facebook.com/posts/1639473982937255/updating-our-open-source-patent-grant/).

src/__mocks__/vendor/third_party/WebComponents.js
8: * subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt
```

* Version bumps to use MIT license

* Add ReactTestRenderer documentations (#10692)

* Add ReactTestRenderer documentations

* Add TestRenderer documentations

* TestRenderer is not experiment

* Add a link for jsdom

* Use ES Modules syntax

* Twaek

* Add a Link component

* Use Jest assertions

* Move a documentation for createNodeMock to Idea section

* Renamed

* Tweak

* Rename

* More explicit

* Add a usecase for createNodeMock

* Add changelog for 15.6.2

* Add 15.6.2 blog post to master

* Add Nate to authors on master

* Bump object-assign patch range to match main package.json

* Flow should ignore node_modules/create-react-class

* Update error codes

* Update CHANGELOG for React 16

* v16.0.0

* Doc updates for React 16 + blog post (#10824)

* Doc updates for React 16 + blog post

* Add link to Sophie's post

* Fix React links on the website (#10837)

* Fix React links on the website

* Fix code editor

* Fix code editor, attempt 2

* Doc change for prevContext removal in CDU (#10836)

* Doc change for prevContext removal in CDU

Ref: #8631

* Minor rewording

* Fix note formatting

* React.createPortal is not a function (#10843)

* Update Portals Documentation (#10840)

* Update Portals Documentation

Correct some grammar to be more explicit and clear. Update example CodePen to better match code found in documentation. Update code example styles to match other code examples (ie. 'State and Lifecycle', 'Handling Events').

* Clean up comment to be accurate to example

There was a small comment overlooked when reviewing the documentation. This fixes it to be accurate to the example as well as grammatically correct.

* Update portals.md

* More fixes

* Update name of property initializer proposal (#10812)

The proposal for property initializers is called [Public Class Fields](https://tc39.github.io/proposal-class-public-fields/) now (part of the combined [Class Fields](https://github.com/tc39/proposal-class-fields) proposal).

* Fix portal link (#10845)

* Update docs for React 16 (#10846)

* Minor doc edit

* Rename urls
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 4, 2017

Member

16 is out.
Remaining items moved to #11088.

Member

gaearon commented Oct 4, 2017

16 is out.
Remaining items moved to #11088.

@gaearon gaearon closed this Oct 4, 2017

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