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

Fix passing symbols and functions to textarea #13362

Merged
merged 11 commits into from Aug 14, 2018
Merged

Fix passing symbols and functions to textarea #13362

merged 11 commits into from Aug 14, 2018

Conversation

@raunofreiberg
Copy link
Contributor

@raunofreiberg raunofreiberg commented Aug 10, 2018

Somewhat related to #11734. Builds on top of @nhunzaker's work on ReactDOMFiberInput

Reproduced cases that work incorrectly (?) as of today (https://codesandbox.io/s/pj40nrp5px):

return (
  <div>
    <textarea defaultValue={() => {}} /> // Stringified into "function defaultValue() {}"
    <textarea value={() => {}} /> // Stringified into "function value() {}"
    <textarea defaultValue={Symbol("test")} /> // TypeError: Cannot convert a Symbol value to a string
    <textarea value={Symbol("test")} /> // TypeError: Cannot convert a Symbol value to a string
  </div>
);
@raunofreiberg raunofreiberg changed the title Ignore symbols functions textarea Fix passing symbols and functions to textarea Aug 10, 2018
Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Awesome! Thank you for the PR. I think @nhunzaker should also take a look because it's similar to his work in #11741. And maybe we want this for select as well?

We probably also need to check if we have to update the attribute snapshots: https://github.com/facebook/react/tree/master/fixtures/attribute-behavior

Loading

@@ -20,6 +20,8 @@ describe('ReactDOMTextarea', () => {
let renderTextarea;

beforeEach(() => {
jest.resetModules();
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 10, 2018

Choose a reason for hiding this comment

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

Is there any particular reason for this addition? I think the only reason we do that is when we have tests that require multiple separate versions of React at the same time.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

Running the tests added in this PR were failing unless I ran resetModules before each.

image

Although running them 1 by 1 via fit seemed to fix the issue.

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 10, 2018

Choose a reason for hiding this comment

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

Ah, right, makes sense. Otherwise the warning will only pop up once 👍

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

Btw, thanks for the feedback. I really appreciate it 🙂

Loading

@@ -116,7 +117,9 @@ export function initWrapperState(element: Element, props: Object) {
}

node._wrapperState = {
initialValue: '' + initialValue,
initialValue: getSafeValue(
props.value != null ? props.value : initialValue,
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 10, 2018

Choose a reason for hiding this comment

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

This seems to be different than the previous implementation. Do you mind explaining why? 🙂

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

Ah, you're right. This has to be a mistake. I took influence from ReactDOMFiberInput and the files got oddly similar under cognitive load.

Instead, it can simply be getSafeValue(initialValue), I think :-)

Thanks!

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

initialValue already references props.value on L:83 so this logic is redundant.

Loading

ReactDOM.render(<textarea defaultValue={Symbol('foobar')} />, container);
const node = container.firstChild;

// TODO: add warnings once defaultValue is compatible for them
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 10, 2018

Choose a reason for hiding this comment

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

I don't quite understand this comment. @nhunzaker I think this is copied over from #11741 - Can you point me to something so I can read up on that?

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

Sorry, should've been more thorough. I've added this comment as a reminder for future references once this PR is good to go. Currently there are no warnings for reserved props in React and hence the TODOs in @nhunzaker's PR and this one :-)

Loading

Copy link
Contributor

@nhunzaker nhunzaker Aug 10, 2018

Choose a reason for hiding this comment

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

I know I'm the first offender here (what is // TODO: we should warn here. even? 😿 ), but could you change this comment to something like:

// TODO: defaultValue is a reserved prop and is not validated. Check warnings when they are.

Loading

@@ -126,7 +127,7 @@ export function updateWrapper(element: Element, props: Object) {
if (value != null) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
const newValue = '' + value;
const newValue = getSafeValue(value);
Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

I have a question about this change - could someone please be more specific about which cases require the casting done previously ('' + value) and what exactly breaks by removing it from here and the previous lines?

Loading

Copy link
Contributor

@nhunzaker nhunzaker Aug 10, 2018

Choose a reason for hiding this comment

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

My first recollection is that we don't need to convert these to string values, but we didn't update all of the locations because it wasn't specific to the PR.

Inputs went through a lot of fixes as we ironed out all of the edge cases for special input types on Chrome/Safari, so to some degree, touching more than we needed to felt scary 🙈.

I think it would be good to take a second look at this.

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Looks good at first pass. I'm doing some travel today, but I'll be able to give a more thorough review once I'm settled.

Thanks for sending this out!

Loading

@@ -126,7 +127,7 @@ export function updateWrapper(element: Element, props: Object) {
if (value != null) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
const newValue = '' + value;
const newValue = getSafeValue(value);

// To avoid side effects (such as losing text selection), only set value if changed
if (newValue !== node.value) {
Copy link
Contributor

@nhunzaker nhunzaker Aug 10, 2018

Choose a reason for hiding this comment

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

Ah right. We need the new value to be a string for comparison (edit) because the value always reports as a string.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

That does make sense to me. Is there a particular reason though why the casting is done by concatenation instead of e.g. String(value)? Because with Symbols it produces a TypeError when trying to concat a Symbol and a string.

Moreover, if a textarea receives for i.e a function as its value and we concat it into a string, then getSafeValue would fail its check and append it as a valid value.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 10, 2018

Choose a reason for hiding this comment

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

Maybe we could add a similar check like in ReactDOMFiberInput?

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 10, 2018

Choose a reason for hiding this comment

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

Is there a particular reason though why the casting is done by concatenation instead of e.g. String(value)?

There were some performance concerns about String(): #11741 (comment)

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 11, 2018

Choose a reason for hiding this comment

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

because the value always reports as a string

I was wondering why Flow did not catch that and found the issue in the getSafeValue() function. Not sure if we want the fix since it adds some complexity but it's nice to know that all code paths indeed work. Check out: #13367

Loading

philipp-spiess added a commit to philipp-spiess/react that referenced this issue Aug 11, 2018
This is an attempt in improving the soundness for the safe value cast
that was added in facebook#11741. We want this to avoid situations like [this
one](facebook#13362 (comment))
where we need to remember why we have certain type casts. Additionally
we can be sure that we only cast safe values to string.

The problem was `getSafeValue()`. It used the (deprecated) `*` type to
infer the type which resulted in a passing-through of the implicit `any`
of the props `Object`. So `getSafeValue()` was effectively returning
`any`.

Once I fixed this, I found out that Flow does not allow concatenating
all possible types to a string (e.g `"" + false` fails in Flow). To
fix this as well, I've opted into making the SafeValue type opaque and
added a function that can be used to get the string value. This is sound
because we know that SafeValue is already checked.

I've verified that the interim function is inlined by the compiler and
also looked at a diff of the compiled react-dom bundles to see if I've
regressed anything. Seems like we're good.
philipp-spiess added a commit that referenced this issue Aug 12, 2018
* Improve soundness of ReactDOMFiberInput typings

This is an attempt in improving the soundness for the safe value cast
that was added in #11741. We want this to avoid situations like [this
one](#13362 (comment))
where we need to remember why we have certain type casts. Additionally
we can be sure that we only cast safe values to string.

The problem was `getSafeValue()`. It used the (deprecated) `*` type to
infer the type which resulted in a passing-through of the implicit `any`
of the props `Object`. So `getSafeValue()` was effectively returning
`any`.

Once I fixed this, I found out that Flow does not allow concatenating
all possible types to a string (e.g `"" + false` fails in Flow). To
fix this as well, I've opted into making the SafeValue type opaque and
added a function that can be used to get the string value. This is sound
because we know that SafeValue is already checked.

I've verified that the interim function is inlined by the compiler and
also looked at a diff of the compiled react-dom bundles to see if I've
regressed anything. Seems like we're good.

* Fix typo
@@ -11,6 +11,7 @@ import invariant from 'shared/invariant';
import warning from 'shared/warning';

import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes';
import getSafeValue from './getSafeValue';

let didWarnValDefaultVal = false;

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

The type of initialValue in the _wrapperState below should probably be SafeValue now, just like we did for input elements.

Edit: Nevermind you have to push your changes first 🙈

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 12, 2018

Choose a reason for hiding this comment

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

Just pushed my changes - I hope they make sense and I took the right approach :-)

Loading

@@ -54,7 +55,7 @@ export function getHostProps(element: Element, props: Object) {
...props,
value: undefined,
defaultValue: undefined,
children: '' + node._wrapperState.initialValue,
children: safeValueToString(getSafeValue(node._wrapperState.initialValue)),
Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 12, 2018

Choose a reason for hiding this comment

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

Can we just do safeValueToString(node._wrapperState.initialValue) instead here?

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

Yes, this seems correct 👍 . _wrapperState is only set once and that is already a SafeValue.

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Thanks for updating the PR against my changes so quickly 🙂 If you want, you can take a look at as well.

Loading

@@ -54,7 +55,7 @@ export function getHostProps(element: Element, props: Object) {
...props,
value: undefined,
defaultValue: undefined,
children: '' + node._wrapperState.initialValue,
children: safeValueToString(getSafeValue(node._wrapperState.initialValue)),
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

Yes, this seems correct 👍 . _wrapperState is only set once and that is already a SafeValue.

Loading

@@ -107,7 +109,7 @@ export function initWrapperState(element: Element, props: Object) {
children = children[0];
}

defaultValue = '' + children;
defaultValue = getSafeValue(children);
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

defaultValue was a string before and is a SafeValue now. I'm not sure if this causes issues with the equality test below.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 13, 2018

Choose a reason for hiding this comment

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

Should we stringify here as well, just to be safe?

Loading

// To avoid side effects (such as losing text selection), only set value if changed
if (newValue !== node.value) {
node.value = newValue;
if ((value: any) !== node.value) {
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

We compared this against a string before and now it's a SafeValue. Is this intended? I think this means when value is a non-string (i.e a number) it will always update node.value since the strict equality will always return false.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 12, 2018

Choose a reason for hiding this comment

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

Hm, right. Would it make more sense if the comparison was props.value !== node.value and use getSafeValue for const newValue?

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess Aug 12, 2018

Choose a reason for hiding this comment

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

That would still false if props.value = 1 and node.value = '1'. I think bringing back const newValue as safeValueToString(value) is more correct.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 12, 2018

Choose a reason for hiding this comment

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

Gotcha. Pushed a change with regards to this.

Loading

philipp-spiess added a commit to philipp-spiess/react that referenced this issue Aug 12, 2018
Following up on the changes I made in facebook#13367, @gaearon suggest that
"safe" could be read as necessary for security. To avoid misleading a
reader, I'm changing the name.

A few names where discussed in the previous PR. I think `ToStringValue`
makes sense since the value itself is not a string yet but an opaque
type that can be cast to a string. For the actual string concatenation,
I've used `toString` now to avoid confusion: `toStringValueToString`
is super weird and it's namespaced anyhow.

Definitely open for suggestions here. :) I'll wait until we wrap up
facebook#13362 and take care of rebase afterwards.
gaearon added a commit that referenced this issue Aug 13, 2018
Following up on the changes I made in #13367, @gaearon suggest that
"safe" could be read as necessary for security. To avoid misleading a
reader, I'm changing the name.

A few names where discussed in the previous PR. I think `ToStringValue`
makes sense since the value itself is not a string yet but an opaque
type that can be cast to a string. For the actual string concatenation,
I've used `toString` now to avoid confusion: `toStringValueToString`
is super weird and it's namespaced anyhow.

Definitely open for suggestions here. :) I'll wait until we wrap up
#13362 and take care of rebase afterwards.
@raunofreiberg
Copy link
Contributor Author

@raunofreiberg raunofreiberg commented Aug 13, 2018

Synced up with the changes from #13376.

Loading

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Changes look good from my end.

Loading

@raunofreiberg
Copy link
Contributor Author

@raunofreiberg raunofreiberg commented Aug 14, 2018

Rebased and fixed conflict derived from #13361.

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Looks great already! Just one comment.

Loading

@@ -110,7 +112,7 @@ export function initWrapperState(element: Element, props: Object) {
children = children[0];
}

defaultValue = '' + children;
defaultValue = getToStringValue(children);
Copy link
Contributor

@philipp-spiess philipp-spiess Aug 14, 2018

Choose a reason for hiding this comment

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

Can you find out if we really need to convert to ToStringValue here?

I think we should be able to just pass children directly as the defaultValue since we convert to the ToStringValue down below anyway.

Loading

Copy link
Contributor Author

@raunofreiberg raunofreiberg Aug 14, 2018

Choose a reason for hiding this comment

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

You're right, this does seem redundant. Removed it, thanks!

Loading

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

This looks perfect now. Thank you very much 🙂 If you want a follow-up task, you could work on the same fix for elements.

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 14, 2018

Taking a quick pass to verify this with the fixtures, but this looks good.

Loading

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Aug 14, 2018

Thanks!

Loading

@nhunzaker nhunzaker merged commit d04d03e into facebook:master Aug 14, 2018
1 check passed
Loading
@raunofreiberg
Copy link
Contributor Author

@raunofreiberg raunofreiberg commented Aug 14, 2018

My pleasure 🙂

Loading

@raunofreiberg raunofreiberg deleted the ignore-symbols-functions-textarea branch Aug 14, 2018
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* Improve soundness of ReactDOMFiberInput typings

This is an attempt in improving the soundness for the safe value cast
that was added in facebook#11741. We want this to avoid situations like [this
one](facebook#13362 (comment))
where we need to remember why we have certain type casts. Additionally
we can be sure that we only cast safe values to string.

The problem was `getSafeValue()`. It used the (deprecated) `*` type to
infer the type which resulted in a passing-through of the implicit `any`
of the props `Object`. So `getSafeValue()` was effectively returning
`any`.

Once I fixed this, I found out that Flow does not allow concatenating
all possible types to a string (e.g `"" + false` fails in Flow). To
fix this as well, I've opted into making the SafeValue type opaque and
added a function that can be used to get the string value. This is sound
because we know that SafeValue is already checked.

I've verified that the interim function is inlined by the compiler and
also looked at a diff of the compiled react-dom bundles to see if I've
regressed anything. Seems like we're good.

* Fix typo
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
Following up on the changes I made in facebook#13367, @gaearon suggest that
"safe" could be read as necessary for security. To avoid misleading a
reader, I'm changing the name.

A few names where discussed in the previous PR. I think `ToStringValue`
makes sense since the value itself is not a string yet but an opaque
type that can be cast to a string. For the actual string concatenation,
I've used `toString` now to avoid confusion: `toStringValueToString`
is super weird and it's namespaced anyhow.

Definitely open for suggestions here. :) I'll wait until we wrap up
facebook#13362 and take care of rebase afterwards.
TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* refactor: move getSafeValue to separate file

* fix(?): ReactDOMFiberTextarea sanitization for symbols and functions

* tests: add TODOs for warnings

* fix: restore accidentally removed test

* fix: remove redundant logic for initialValue

* refactor: integrate SafeValue typings into textarea

* fix: restore stringified newValue for equality check

* fix: remove getSafeValue from hostProps

* refactor: SafeValue -> ToStringValue

* refactor: update TODO comment in test file

* refactor: no need to convert children to ToStringValue
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants