This repository was archived by the owner on Jul 19, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 41
Add notes for May 5 #13
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| ## May 5 ([discuss](https://github.com/reactjs/core-notes/pull/13)) | ||
|
|
||
| ### Attendees | ||
|
|
||
| * [Ben](https://twitter.com/soprano) (React) | ||
| * [Dan](https://twitter.com/dan_abramov) (React) | ||
| * [Jim](http://github.com/jimfb) (React) | ||
| * [Paul](https://twitter.com/zpao) (React) | ||
| * [Sebastian](https://twitter.com/sebmarkbage) (React) | ||
| * [Shayne](https://github.com/shayne) (React Native) | ||
| * [Tom](https://twitter.com/tomocchino) (React) | ||
|
|
||
| ### ReactPerf Rewrite | ||
|
|
||
| * [Dan](https://twitter.com/dan_abramov) got his rewrite of `ReactPerf` [merged into React master](https://github.com/facebook/react/pull/6647). | ||
| * It is mostly functionally identical to the old one but is written in a more maintainable way. | ||
| * At Facebook, we don’t use the console `ReactPerf` API much so we’ll need community help testing this. | ||
| * There is a corresponding [React Native PR](https://github.com/facebook/react-native/pull/7283) that depends on these changes. | ||
| * The change removes `ReactPerf.measure()` wrappers and instead emits events (e.g. `onBeginLifeCycleTimer`). | ||
| * Related PRs: [#6549](https://github.com/facebook/react/pull/6549), [#6612](https://github.com/facebook/react/pull/6612), [#6633](https://github.com/facebook/react/pull/6633), [#6046](https://github.com/facebook/react/pull/6046). | ||
| * It will most likely go into `15.1.0-alpha` early next week. | ||
|
|
||
| ### TestUtils and Enzyme | ||
|
|
||
| * Airbnb’s [Enzyme](https://github.com/airbnb/enzyme) is well maintained and supported. | ||
| * Our [`TestUtils`](https://facebook.github.io/react/docs/test-utils.html) are not in a great shape, and most people prefer Enzyme. | ||
| * Should Enzyme become the official `TestUtils`? | ||
|
|
||
| #### Challenges | ||
|
|
||
| * Ideally we want test runner to be a separate renderer. | ||
| * Tests should run consistently and often synchronously so we don’t want to run them with [incremental reconciler](https://github.com/facebook/react/issues/6170). | ||
| * We have no official renderer APIs yet so without `TestUtils` Enzyme would have to rely on React internals. | ||
|
|
||
| #### Risks | ||
|
|
||
| * If Enzyme is abandoned, we will have to maintain it. | ||
| * Not a big deal because this is pretty much our situation with `TestUtils` now. | ||
| * Recommending it as an official solution might raise questions about “official solutions” to other problems. | ||
| * (e.g. routing, state management) | ||
| * We don’t have guidelines for recommending something as an official solution. | ||
| * No conclusion yet, needs more discussion. | ||
|
|
||
| ### Server Rendering | ||
|
|
||
| * After thinking more about incremental reconciliation, [Sebastian](https://twitter.com/sebmarkbage) has some renewed interest in server rendering. | ||
| * Whatever feature server rendering wants, it’s a good feature to have on the client as well. | ||
| * For example, if you have a component that’s unlikely to change: | ||
| * Currently you have all those React internal instances in memory. | ||
| * But if you remove those, and are able to recover… | ||
| * It’s just like reviving a server rendered tree on the client. | ||
| * Current behavior for rehydration after server rendering: | ||
| * It re-renders everything on the client to a string to verify the checksum, then throws it out. | ||
| * If it’s the same, and we need the node or instance for any reason, at that point we climb the tree and figure out what instance it corresponds to lazily. | ||
| * It doesn’t fix any inconsistencies it finds as it traverses back up. | ||
| * This is what [@aickin](https://github.com/aickin) is trying to change in his [pull request](https://github.com/facebook/react/pull/6618). | ||
| * What does [#6618](https://github.com/facebook/react/pull/6618) help with? | ||
| * We can loosen constraints on validation. | ||
| * Markup doesn’t have to be character for character equivalent. | ||
| * [Ben](https://twitter.com/soprano) has [concerns](https://github.com/facebook/react/pull/6618#issuecomment-217321531) about it. | ||
|
|
||
| ### CSS Fallback Values | ||
|
|
||
| * We don’t have a way of letting people specify fallback values for vendor prefixes. | ||
| * People ask for something like this: `display: ['-webkit-box', '-moz-box', '-ms-flexbox', '-webkit-flex']`. | ||
| * Using arrays for this precludes supporting something like `margin: [0, 0]` in the future. | ||
| * We could use an opaque data structure for this, e.g. `ReactDOM.CSS.multi('-webkit-box', '-moz-box', ...)`. | ||
| * There’s a proof of concept in [#6701](https://github.com/facebook/react/pull/6701). | ||
| * Still needs more bikeshedding on the name but the idea is sound. | ||
| * We might want to look at autoprefixing again the next time we approach inline styles in the future. | ||
|
|
||
| ------------ | ||
|
|
||
| Please feel free to discuss these notes in the [corresponding pull request](https://github.com/reactjs/core-notes/pull/13). | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would it mean to become the "official" test utils?
There is more than one test helpers lib out there, blessing a single one would be a bit harsh on the others.
I'd be in favour of trimming down TestUtils functions and linking to a community list of tools which build on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would mean that Enzyme is linked to in the documentation and we tell people to use it rather than to
scryRenderedComponentsOfType. For us, it would also mean no longer shipping theTestUtilsas we don’t use them that much at Facebook.Whether either of this is something we want to do, or not, is unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having core provide testing primitives and leaving userland to layer on top is a good thing, but I'd be wary of recommending only a single lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the reasons why there is no decision on this yet. It's a tricky balance. “Testing primitives” is also vague: we do provide them today (
TestUtils) but we are not sure that this is the primitives we want to expose. As noted in text, we’d rather see test runner as a custom React renderer. In this version of the future, React does not need to expose any hooks at all because the custom renderer has full control over components. This is probably where we want to go but the obstacle is we don't have a public API for renderers, and the internal APIs will be changing because of the work on incremental reconciler. Therefore writing custom renderers will be turbulent in the coming months. This was the reason React Native renderer code was pulled into React codebase—to allow faster iteration on the internals.So none of this is clear yet. While
TestUtilsexist the way they do today, it doesn’t quite make sense to tell people to use something else yet. However in the future, it might make sense for Enzyme to become more tightly coupled with implementation details of React, at least until the renderer API shakes out. If this happens, and it gives us a way to deprecateTestUtilsin the future, we’ll likely need to point to something—and I would expect that it’s likely we won’t have other test renderers at that point because creating a renderer right now is somewhat brittle and requires quite a bit of effort. In that case, maybe, recommending Enzyme officially might be sensible.But we don’t know much yet. These are just thoughts aloud. We are talking through different options and did not choose any particular strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the shallow rendering capability remain a part of core react - whether that's a custom renderer or not shouldn't make a lot of difference to a consumer.
Enzyme's main utility is in replacing the
find/scrymethods with ones which work consistently when rendering to DOM, String or Shallow. Having that library be the only place where a shallow renderer exists would be disappointing.If moving the shallow rendering code out of core is a goal, hopefully the community (myself included) will find a way to keep it decoupled from the test assertion / node traversal stuff.
I hadn't realised enzyme was already prominently featured on the TestUtils docs page. This is a bit disappointing to me, as although I have nothing against enzyme, it pretty much kills any hope similar libraries have of getting takeup when exploring that space.
Anyway, to summarise my thoughts on this:
(find|scry)*This is driven from the fact I have a shallow-rendering based testing lib which is a couple of months older than enzyme and has ~40% of the monthly npm download numbers enzyme does. I'd like to see even more competition / exploration in this space, and having core bless a particular lib could stifle that.