Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates ViewConfig types to delegate isInAParentText context #29872

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

rozele
Copy link
Contributor

@rozele rozele commented Jun 12, 2024

Summary

Now that HostContext determination for Fabric is a DEV-only behavior, we can move the HostContext determination to resolve from the ViewConfig for a given type. Doing this will allow arbitrary types to register themselves as potential parents of raw text string children. This is the first of two diffs for react as we'll:

  1. Add the new property to the ViewConfig types
  2. Update React Native to include the supportsRawText property for RCTText, RCTVirtualText, AndroidTextInput, etc.
  3. Switch the behavior of react to read from the ViewConfig rather than a static list of types.

How did you test this change?

  • yarn test
  • yarn test --prod
  • Pulled change into react-native, added supportsRawText props to RCTText, RCTVirtualText, etc. ViewConfigs and confirmed everything type checks.

Copy link

vercel bot commented Jun 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 14, 2024 5:47pm

@react-sizebot
Copy link

react-sizebot commented Jun 12, 2024

Comparing: 3154ec8...224892e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB = 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 583.08 kB 583.08 kB = 103.21 kB 103.21 kB
facebook-www/ReactDOM-prod.modern.js = 571.52 kB 571.52 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/shims/ReactNativeTypes.js +0.67% 8.67 kB 8.73 kB +0.52% 2.32 kB 2.33 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 224892e

Now that HostContext determination for Fabric is a DEV-only behavior, we can move the HostContext determination to resolve from the ViewConfig for a given type. Doing this will allow arbitrary types to register themselves as potential parents of raw text string children. This is the first of two diffs for react as we'll:

1. Add the new property to the ViewConfig types
2. Update React Native to include the `supportsRawText` property for `RCTText`, `RCTVirtualText`, `AndroidTextInput`, etc.
3. Switch the behavior of react to read from the ViewConfig rather than a static list of types.
@rozele rozele force-pushed the parentTextContext branch from aafdf65 to 224892e Compare June 14, 2024 17:45
@rozele rozele marked this pull request as ready for review June 14, 2024 17:47
@rozele rozele changed the title Adds option to resolve raw text support from view configs Updates ViewConfig types to delegate isInAParentText context Jun 14, 2024
javache
javache previously approved these changes Jun 19, 2024
Copy link
Member

@javache javache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this change incremental? Eg check for supportsRawText, and if not set or false, check the existing list of component names? That way we have to do less juggling to make this change atomic.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests? I know that this just updates the types, but there should be some sort of tests for this behavior before we start implementing it. And the existing tests that are there are currently passing, but should fail without the changes @javache mentioned.

Here's where I would add them:

it('should console error for text not inside of a <Text> ancestor', async () => {

@rickhanlonii rickhanlonii requested a review from javache June 19, 2024 13:28
@rickhanlonii rickhanlonii dismissed javache’s stale review June 19, 2024 13:29

Need to add tests

@rozele
Copy link
Contributor Author

rozele commented Jun 20, 2024

Could we make this change incremental? Eg check for supportsRawText, and if not set or false, check the existing list of component names? That way we have to do less juggling to make this change atomic.

@javache Not seeing the value in this, it's just as easy to:

  1. expose the types in React
  2. make the change in RN to use the types
  3. Switch to using ViewConfig in React

As it is to:

  1. Expose the types and using ViewConfig with fallback in React
  2. Make the change in RN to use the types
  3. Remove the fallback list in React

@rozele
Copy link
Contributor Author

rozele commented Jun 20, 2024

@rickhanlonii if we are just adding types in this change, there's not really anything to test. The subsequent change after RN is updated to consume the new types will need to update the existing tests, which appear to have sufficient coverage.

@rickhanlonii
Copy link
Member

here's not really anything to test

Yeah ok sure, we can add tests when we do step 3 to actually consume the prop in React. If you're going this route though, it would be easier to just add the props to the config with a FlowFixMe and remove the FlowFixMe after the behavior change lands.

Then you could add the type, the behavior, and the tests all in one React PR.

@rozele rozele merged commit a5cc797 into facebook:main Jul 11, 2024
44 checks passed
@rozele rozele deleted the parentTextContext branch July 11, 2024 16:47
github-actions bot pushed a commit that referenced this pull request Jul 11, 2024
<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

Now that HostContext determination for Fabric is a DEV-only behavior, we
can move the HostContext determination to resolve from the ViewConfig
for a given type. Doing this will allow arbitrary types to register
themselves as potential parents of raw text string children. This is the
first of two diffs for react as we'll:

1. Add the new property to the ViewConfig types
2. Update React Native to include the `supportsRawText` property for
`RCTText`, `RCTVirtualText`, `AndroidTextInput`, etc.
3. Switch the behavior of react to read from the ViewConfig rather than
a static list of types.

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

- yarn test
- yarn test --prod
- Pulled change into react-native, added `supportsRawText` props to
RCTText, RCTVirtualText, etc. ViewConfigs and confirmed everything type
checks.

DiffTrain build for commit a5cc797.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants