Allow third party react-native components to render strings #13211
Conversation
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
|
Just to add a bit of weight to the PR. We can't use styled-components with react-native anymore (styled-components/styled-components#1974). Looks like this |
Details of bundled changes.Comparing: 34348a4...76f43e6 schedule
Generated by 🚫 dangerJS |
|
Please merge this PR! It is causing a lot of problems with some third party libraries. |
|
@hramos can you take a look into this? |
|
Please merge this PR. Waiting for this merge from a long time. |
|
What's holding this PR up from merging? Looks good to go |
|
I am facing issue in react native 0.57. Please merge this PR. |
|
@rkdavidson this being the React repo, I defer to the @facebook/react-core team. |
|
Ah @hramos thanks, forgot about the general team @ handle. |
Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming.
…14065) * Fixed `treeBaseDuration` by propagating its value from the suspended tree to the Fragment React temporarily wraps around it when showing the fallback UI. * Fixed `actualDuration` by recording elapsed profiler time in the event of an error. * Fixed `actualDuration` in concurrent mode by propagating the time spent rendering the suspending component to its parent. Also updated ReactSuspensePlaceholder-test.internal to cover these new cases.
* Parse build script type and package names This ensures that `yarn build core dom` includes DOM. It also ensures that spaces like `yarn build "core, dom"` doesn't build EVERYTHING. * Get rid of label in bundles config Instead we just use the name from entry using fuzzy search. There is one special case. If you put in `/index` or `/index.js`. That allows to build things like `react/index` to only build isomorphic where as `react` would build everything. Or `react-dom/index` to exclude the server renderers. * Instead of matching `/index.js` just append it to the search string That way things like `yarn build react/` works too.
* Refactor ESLint configuration to enable better IDE integration * Minor tweaks
* Add failing test for ping on unmounted component We had a test for this, but not outside of concurrent mode :) * Don't warn if an unmounted component is pinged
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught #14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
Oopsie! This could have been avoided if our types were modeled correctly with Flow (using a disjoint union). Fuzz tester didn't catch it because it does not generate cases where a Suspense component mounts with no children. I'll update it.
* Revert #14756 changes to ReactFiberScheduler This PR introduced some bugs in concurrent mode during internal testing. Until we figure out a proper solution, I'm going to try reverting it. Not totally certain this is sufficient to unbreak the bugs we found, but I'm using this branch to determine that. * Add back commented out Scheduler import With a note not to use named imports next time we import Scheduler in this module.
These used to be different things, but now ReactNoop.yield merely re-exports Scheduler.yieldValue, so let's get rid of it.
#15005) * fix(auto-version-update): update root package version while publishing * fix(remove-version): remove version field from package json
* Warn on mount when deps are not an array * Check other Hooks * I can't figure out how to fix error/warning nesting lint But it doesn't really matter much because we test other cases in the other test.
* Treat functions that don't capture anything as static * Fix comment
* Import Scheduler directly, not via host config We currently schedule asynchronous tasks via the host config. (The host config is a static/build-time dependency injection system that varies across different renderers — DOM, native, test, and so on.) Instead of calling platform APIs like `requestIdleCallback` directly, each renderer implements a method called `scheduleDeferredCallback`. We've since discovered that when scheduling tasks, it's crucial that React work is placed in the same queue as other, non-React work on the main thread. Otherwise, you easily end up in a starvation scenario where rendering is constantly interrupted by less important tasks. You need a centralized coordinator that is used both by React and by other frameworks and application code. This coordinator must also have a consistent API across all the different host environments, for convention's sake and so product code is portable — e.g. so the same component can work in both React Native and React Native Web. This turned into the Scheduler package. We will have different builds of Scheduler for each of our target platforms. With this approach, we treat Scheduler like a built-in platform primitive that exists wherever React is supported. Now that we have this consistent interface, the indirection of the host config no longer makes sense for the purpose of scheduling tasks. In fact, we explicitly do not want renderers to scheduled task via any system except the Scheduler package. So, this PR removes `scheduleDeferredCallback` and its associated methods from the host config in favor of directly importing Scheduler. * Missed an extraneous export
…on is a dependency (#15026) * Warn about bare function deps and suggest moving or useCallback * Clearer wording
* A clearer message for props destructuring where applicable * Add line number to the "move function" message * Add a hint for how to fix callbacks from props * Simplify code and harden tests * Collect all dependency references for better warnings * Suggest updater or reducer where appropriate
…ing (#15055) * Use first letter in setCount(c => ...) suggestion In-person testing showed using original variable name confuses people. * Warn about async effects
* Convert ReactSuspensePlaceholder tests to use noop Instead of the test renderer, since test renderer does not support running in persistent mode. * Run Placeholder tests in persistent mode, too * Fix Flow and lint * Hidden text instances should have correct host context Adds a test for a subtle edge case that only occurs in persistent mode. * createHiddenTextInstance -> cloneHiddenTextInstance This sidesteps the problem where createHiddenTextInstance needs access to the host context.
|
wow...wanted to use react-native-text-gradient but it is dependent on this being merged in. Kind of bummer we are coming up on a year and no merge yet |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Any update on what's holding this PR up? |
6 tasks
|
The approach taken by this PR is not something we'd go with. It adds checks in a very hot path and introduces a new public API which isn't something we do lightly. I also don't think this API makes sense for this use case. It should "just work", not require you to annotate something with a special tag. If this is still a problem please file a new issue where we can discuss this. |
|
Also all the "please merge" comments here weren't helpful. What would have been helpful is filing a new issue with a concrete problem. Because these comments were all on a PR that isn't shippable, we missed this whole discussion. |
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
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.
After introducing new invariant checks in react-native 0.56.0 release, third party native components that can render strings became broken - e.g. iyegoroff/react-native-text-gradient#11
This PR allows third party components to render strings if they have static property
canRenderStringset totrue.Usage: