Moar attribute tests #10509

acdlite · 2017-08-22T20:01:46Z

@acdlite and @flarnie, after chatting with @sebmarkbage, have written in code what we would expect or assume the updated "unknown props" behavior would be.

This is open to discussion - this test demonstrates which behaviors we expect and what differs in the current implementation. We should determine which failing tests should be changed and which ones indicate bugs, and then update this PR.

flarnie · 2017-08-22T21:51:44Z

Specific questions:

For true and false passed to unknown attributes, should we convert them to a string or remove the attribute?
Do we want to warn for NaN, symbols, objects, and functions?
Currently we try to coerce a Symbol to a string and it throws. Should we continue doing this? Probably we could change our expectation here, since that behavior is consistent with known props.
Do we want to put stringified functions in the DOM? We had expected it would, because that is what happens with known attributes, but it's also good to avoid this, since it could introduce security issues. So maybe we should change our expectation for this behavior across all properties.

Lastly:

How much does 2999811 change the behavior for known props and close-matches-for-known-props (like class and classname)? If there are changes there we should figure out the new expectations for those behaviors too.

nhunzaker · 2017-08-22T21:55:17Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+        }
+
+        // TODO: this does not warn. We think it should.
+        testCoerceToString(NaN);


This is curious because we check for NaN. Is it possible that the UnknownPropertyHook is caching a warning for the unknown prop?

sebmarkbage · 2017-08-22T21:56:38Z

@acdlite I thought we had warn + remove for anything blacklisted, such as symbols, functions and blacklisted attribute names?

nhunzaker · 2017-08-22T21:59:32Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+
+        // TODO: either change what we expect or change our implementation
+        // this throws "TypeError: Cannot convert a Symbol value to a string"
+        // testCoerceToString(Symbol('foo'));


I didn't realize Symbol('foo') + '' raised an exception. Huh...

I think this is correct. At least, assigning an attribute with a Symbol manually raises an exception:

I think this makes sense - like I said, we throw the same error for known attributes receiving a Symbol in 15.*. I can update the test.

nhunzaker · 2017-08-22T22:00:34Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+
+        // TODO: either change what we expect or change our implementation
+        // this does not set it to the stringified function.
+        testCoerceToString(() => 'foo');


This should cause the attribute to be removed and raise a warning. Is it possible that the unknown prop is already in the warning cache at this point?

Worth checking - I'll update this to include something like https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L26-L32 and that should clear the warning cache, right?

nhunzaker · 2017-08-22T22:00:58Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+        testCoerceToString(() => 'foo');
+
+        // TODO: this does not warn. We think it should.
+        testCoerceToString({hello: 'world'});


Same question here. This should warn and not assign an attribute.

Will try adding the jest.resetModules stuff in a beforeEach and see if the warnings show up.

Oh nevermind - we already have that in the test.

Not sure how else to force the warning cache to clear?

Hmm. I'm not 100% sure how jest.resetModules works with common js. What if we exposed a method on the UnknownDOMPropertyHook to reset the cache?

acdlite · 2017-08-22T22:01:25Z

@sebmarkbage We changed our minds after talking to Flarnie and Ben, the reasoning being that it'll make it easier to migrate / land a sync.

nhunzaker

I'm curious about warning caching. We've stretched the UnknownDOMPropertyHook to also include bad data types. Maybe we should move the data validation warnings to before the bail-out if an attribute has already been warned about.

sebmarkbage · 2017-08-22T22:02:30Z

@acdlite We change our minds from throw to warn but not to stringify. Stringify makes it harder to land than remove.

acdlite · 2017-08-22T22:02:43Z

Oh wait you're right, the change was that previously we said this this would throw but now it only warns. But should still remove.

flarnie · 2017-08-22T22:04:25Z

Oh wait you're right, the change was that previously we said this this would throw but now it only warns. But should still remove.

Agreed - I think we might have just misread the notes from yesterday's discussion. Makes sense to remove for functions and symbols.

acdlite · 2017-08-22T22:04:26Z

Yeah that was just a mistake we made when filling out the chart. That's what my notes say.

flarnie · 2017-08-22T22:12:41Z

I think this is the only unanswered question:

For true and false passed to unknown attributes, should we convert them to a string or remove the attribute?

flarnie · 2017-08-22T22:37:49Z

Updates:

We uncommented/enabled the failing tests to make this more clear.
How hard would it be to handle the case where a Symbol is passed? We are concerned that folks may be using spread to pass props in, and mistakenly have a Symbol as a value, and then this change would introduce a cryptic and possibly app-breaking error in production. Seems like we would want to warn and remove it, both for known and unknown attributes.

flarnie · 2017-08-22T22:46:12Z

I opened an issue about the case of Symbols - #10512

gaearon · 2017-08-22T23:19:07Z

I think this is the only unanswered question:

For true and false passed to unknown attributes, should we convert them to a string or remove the attribute?

I recall that we wanted to strip them because we only pass numbers, strings, (and as of b923740) objects. My impression is we already have tests for these cases which were part of Nate's original PR. (But I might be wrong.)

How hard would it be to handle the case where a Symbol is passed?

Shouldn't be hard. Let's warn and skip them. I can do tomorrow after my plane lands, or maybe Nate can beat me to it.

nhunzaker · 2017-08-22T23:20:57Z

What is the best way for me to work against this test suite? Should I send PRs to @acdlite's fork?

acdlite · 2017-08-22T23:28:23Z

@nhunzaker Flarnie and I are finishing up the tests, then you can just push commits directly to this branch. I'll comment here in a bit once we're ready.

acdlite · 2017-08-22T23:47:09Z

@nhunzaker I believe the tests now match the behavior @sebmarkbage, @flarnie, and I settled on yesterday. Thanks so much for all your work on this.

acdlite · 2017-08-22T23:48:43Z

Also I realize some of these tests are duplicates of tests that already exist... I personally think that's fine, but if you have a better of way of organizing them that's also fine, as long as all the edge cases are accounted for.

acdlite · 2017-08-22T23:50:16Z

Oh and I'll rebase before merging to make sure you get credit for these changes :)

nhunzaker · 2017-08-23T00:28:05Z

Okay. I'll take a look!

nhunzaker · 2017-08-23T00:36:14Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+      testUnknownAttributeAssignment(lol, 'lol');
+      // TODO: add specific expectations about what the warning says
+      // expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(...
+      expectDev(console.error.calls.count()).toBe(1);


Should this warn? I thought we decided to allow toString methods to support libraries like glamor that use custom toString methods, and the ajaxify property found commonly in FB's codebase.

Hmm that's what I recorded in my notes after yesterday's conversation, but I may be mistaken. I though the rationale was that toString could be slow and we should encourage people not to rely on it.

@sebmarkbage

Yeah you're right, Sebastian says (for now at least) we'll omit the warning, since so many people rely on it. We may revisit this in the future.

Cool. Sounds good!

nhunzaker · 2017-08-23T00:56:11Z

src/renderers/dom/shared/DOMProperty.js

-          return true;
-        }
-        var prefix = lowerCased.slice(0, 5);
-        return prefix === 'data-' || prefix === 'aria-';


Just for clarity: we used to only allow booleans for non-flagged attributes on data and aria attributes. Now all attributes can accept booleans, cast to strings.

nhunzaker · 2017-08-23T13:29:39Z

Ah, 👍

flarnie · 2017-08-23T15:49:03Z

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

-      expectDev(console.error.calls.argsFor(0)[0]).toContain(
-        'Warning: Invalid prop `whatever` on <div> tag',
-      );
+      expect(el.getAttribute('whatever')).toBe('true');


This is a case we didn't test or discuss - I think this approach makes sense.

flarnie · 2017-08-23T16:12:29Z

Thanks so much @nhunzaker and @gaearon for all your work on this. I read through the changes and I think it makes sense.

Merge whenever you are ready - I am happy to run a sync that includes this commit. :)

aweary · 2017-08-23T16:34:46Z

If boolean attributes are passed through, can we avoid stringifying them? That way we can support boolean attributes <div foo={true} /> and enumerated attributes that take stringified booleans <div bar="true" />.

flarnie · 2017-08-23T16:40:57Z

"If boolean attributes are passed through, can we avoid stringifying them? "

@aweary I think I need more context to understand this question - it seems like passing them through and avoiding stringifying are mutually exclusive.

The second example, <div bar="true" /> is what I would identify as a string value for an unknown attribute, right?

I think we all agree that passing booleans through is not ideal.

flarnie · 2017-08-23T16:47:31Z

I'm heading to work but would like to merge this soon so that we can pull it into FB's stack for testing. We have kind of a small window when we can merge big changes to FB, sorry about the time pressure.

@aweary If you still have serious concerns about the boolean pass-through behavior then I trust your judgement, let me know and we can back things up. It will take at minimum several hours to get the sync landed.

aweary · 2017-08-23T16:58:32Z

So before this PR, trying to use a boolean value for an unknown attribute would result in a warning and the attribute wouldn't be set. This is because shouldSetAttribute would return false so null would be passed to setValueForAttribute since the attribute isn't a data- or aria- attribute and null values trigger removeAttribute. You can see that behavior on master here.

With the changes in this PR, the attribute is actually set which is why I bring it up here. But the boolean value is stringified so <div unknown-bool={true} /> renders <div unknown-bool="true"> when it should really be rendering <div unknown-bool>. I'm suggesting that we assume unknown attributes with boolean values represent boolean attributes, which per the spec should be set with setAttribute(name, ""). If an unknown attribute is supposed to be an enumerated attribute with a stringified boolean value like spellcheck the user should pass in a stringified boolean like <div spellcheck="true">

Right now users have to know to pass empty strings and null to implement the correct behavior for unknown boolean attributes. This would resolve #9230 as well.

nhunzaker · 2017-08-23T17:24:05Z

@aweary's thoughts makes sense to me, but how do we move forward here? Should we agree one way or the other?

flarnie · 2017-08-23T17:48:12Z

Let's try to come to a consensus.

@aweary I'm sharing some documents with you via quip where the React team has been documenting our discussion of this stuff.

Let's move the discussion to an issue so that this PR doesn't get too much noise.

robdodson · 2017-08-25T20:33:16Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+        },
+      };
+
+      testUnknownAttributeAssignment({hello: 'world'}, '[object Object]');


why do you want to set [object Object] instead of setting a property on the element? A custom element receiving [object Object] can't really do anything with it.

So far, we’ve been focusing on the regular DOM elements so we didn’t change the behavior for custom elements. Given that 16 is imminent we’ll probably have to wait before 17 to change it.

robdodson · 2017-08-25T20:35:02Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+      testUnknownAttributeRemoval(function someFunction() {});
+      expectDev(console.error.calls.count()).toBe(1);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'Warning: Received function for attribute `unknown`. Attributes must ' +


Could you set functions as properties?

gaearon · 2017-09-01T17:29:12Z

Do you want to bring this up to date?

flarnie · 2017-09-01T23:46:20Z

I had not realized this was not merged. Someone should update this - I can do it, unless someone else has already started.

**what is the change?:** Adds tests for the following behavior - - Numbers and booleans should be converted to strings, and not warn - NaN, Symbols, functions, and objects should be converted to strings, and *should* warn Going to add tests for the not-warning behavior in a follow-up. These tests are not entirely passing - we either need to change what we expect or change the behavior. **why make this change?:** Gets everyone on the same page about expected behavior, and codifies it in a maintainable way **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` **issue:** facebook#10399

**what is the change?:** We are testing the behavior of unknown attributes, which has changed since React 15. We want to *not* warn for the following cases - - null - undefined - missing - strings - numbers - booleans **why make this change?:** We want to verify that warnings don't get fired at the wrong time. **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` **issue:** facebook#10399

…warn

I don't think we use this convention anywhere.

**what is the change?:** - booleans don't get stringified - some warnings have changed since we originally wrote this **why make this change?:** The attribute behavior is finalized and now we can test it :D I also found it handy to have a row with a truly unknown attribute, so added "imaginaryFriend". **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` and comparing the tests to the attribute table **issue:** facebook#10399

flarnie · 2017-09-02T01:49:12Z

Tests pass for my locally - feel free to merge if so, otherwise I'll merge on Monday.

gaearon · 2017-09-06T14:53:57Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+          'If this is expected, cast the value to a string.\n' +
+          '    in div (at **)',
+      );
+      expectDev(console.error.calls.count()).toBe(1);


It might be better to put this assertion first because otherwise the previous assertion would probably blow up with Cannot read property 0 of undefined. I haven't verified this though.

gaearon · 2017-09-06T14:54:12Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+          'If this is expected, cast the value to a string.\n' +
+          '    in div (at **)',
+      );
+      expectDev(console.error.calls.count()).toBe(1);


gaearon · 2017-09-06T14:54:26Z

src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js

+      expectDev(console.error.calls.count()).toBe(1);
+    });
+
+    it('coerces objects to strings **and warns**', () => {


Nit: I don't think we use Markdown in test titles anywhere.

* WIP * WIP Add the rest of the tests for what we expect re: unknown attributes **what is the change?:** Adds tests for the following behavior - - Numbers and booleans should be converted to strings, and not warn - NaN, Symbols, functions, and objects should be converted to strings, and *should* warn Going to add tests for the not-warning behavior in a follow-up. These tests are not entirely passing - we either need to change what we expect or change the behavior. **why make this change?:** Gets everyone on the same page about expected behavior, and codifies it in a maintainable way **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` **issue:** facebook#10399 * WIP Add check that we *don't* warn when handling some unknown attributes **what is the change?:** We are testing the behavior of unknown attributes, which has changed since React 15. We want to *not* warn for the following cases - - null - undefined - missing - strings - numbers - booleans **why make this change?:** We want to verify that warnings don't get fired at the wrong time. **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` **issue:** facebook#10399 * ran prettier * Symbols and functions passed to unknown attributes should remove and warn * Abstract tests a bit to make them easier to read * Remove Markdown from test names I don't think we use this convention anywhere. * Add an assertion for NaN warning message * Update ReactDOMAttribute test based on attribute fixture **what is the change?:** - booleans don't get stringified - some warnings have changed since we originally wrote this **why make this change?:** The attribute behavior is finalized and now we can test it :D I also found it handy to have a row with a truly unknown attribute, so added "imaginaryFriend". **test plan:** `yarn test src/renderers/dom/shared/__tests__/ReactDOMAttribute-test.js` and comparing the tests to the attribute table **issue:** facebook#10399 * remove imaginaryFriend to resolve conflict * ran prettier

facebook-github-bot added the CLA Signed label Aug 22, 2017

flarnie requested review from sebmarkbage, nhunzaker and gaearon August 22, 2017 20:59

flarnie requested a review from sophiebits August 22, 2017 21:52

nhunzaker reviewed Aug 22, 2017

View reviewed changes

acdlite force-pushed the moarattributetests branch from 1be8c74 to c12715e Compare August 22, 2017 23:42

nhunzaker reviewed Aug 23, 2017

View reviewed changes

flarnie reviewed Aug 23, 2017

View reviewed changes

flarnie mentioned this pull request Aug 23, 2017

Decide on the desired behavior when a boolean value is passed to an unknown prop. #10521

Closed

robdodson reviewed Aug 25, 2017

View reviewed changes

acdlite and others added 8 commits September 1, 2017 17:19

WIP

ce88037

ran prettier

706118e

Symbols and functions passed to unknown attributes should remove and …

b94b5b7

…warn

Abstract tests a bit to make them easier to read

0464fd5

Remove Markdown from test names

acae10d

I don't think we use this convention anywhere.

Add an assertion for NaN warning message

ce03f1e

flarnie force-pushed the moarattributetests branch from 36e1616 to ce03f1e Compare September 2, 2017 00:55

flarnie added 2 commits September 1, 2017 18:40

remove imaginaryFriend to resolve conflict

b88b1d1

ran prettier

083f47f

flarnie merged commit 47d8702 into facebook:master Sep 5, 2017

gaearon reviewed Sep 6, 2017

View reviewed changes

Moar attribute tests #10509

Moar attribute tests #10509

Conversation

acdlite commented Aug 22, 2017 • edited by flarnie

flarnie commented Aug 22, 2017

Choose a reason for hiding this comment

sebmarkbage commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Aug 22, 2017

nhunzaker left a comment

Choose a reason for hiding this comment

sebmarkbage commented Aug 22, 2017

acdlite commented Aug 22, 2017

flarnie commented Aug 22, 2017

acdlite commented Aug 22, 2017

flarnie commented Aug 22, 2017

flarnie commented Aug 22, 2017

flarnie commented Aug 22, 2017

gaearon commented Aug 22, 2017

nhunzaker commented Aug 22, 2017

acdlite commented Aug 22, 2017

acdlite commented Aug 22, 2017

acdlite commented Aug 22, 2017

acdlite commented Aug 22, 2017

nhunzaker commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Aug 23, 2017

Choose a reason for hiding this comment

flarnie commented Aug 23, 2017

aweary commented Aug 23, 2017

flarnie commented Aug 23, 2017

flarnie commented Aug 23, 2017

aweary commented Aug 23, 2017

nhunzaker commented Aug 23, 2017

flarnie commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Sep 1, 2017

flarnie commented Sep 1, 2017

flarnie commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Aug 22, 2017 •

edited by flarnie

acdlite Aug 23, 2017 •

edited