-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Moar attribute tests #10509
Moar attribute tests #10509
Conversation
Specific questions:
Lastly:
|
} | ||
|
||
// TODO: this does not warn. We think it should. | ||
testCoerceToString(NaN); |
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 curious because we check for NaN. Is it possible that the UnknownPropertyHook is caching a warning for the unknown
prop?
@acdlite I thought we had warn + remove for anything blacklisted, such as symbols, functions and blacklisted attribute names? |
|
||
// TODO: either change what we expect or change our implementation | ||
// this throws "TypeError: Cannot convert a Symbol value to a string" | ||
// testCoerceToString(Symbol('foo')); |
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.
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 this makes sense - like I said, we throw the same error for known attributes receiving a Symbol in 15.*. I can update the test.
|
||
// TODO: either change what we expect or change our implementation | ||
// this does not set it to the stringified function. | ||
testCoerceToString(() => 'foo'); |
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 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?
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.
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?
testCoerceToString(() => 'foo'); | ||
|
||
// TODO: this does not warn. We think it should. | ||
testCoerceToString({hello: 'world'}); |
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.
Same question here. This should warn and not assign an attribute.
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.
Will try adding the jest.resetModules
stuff in a beforeEach
and see if the warnings show up.
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.
Oh nevermind - we already have that in the test.
Not sure how else to force the warning cache to clear?
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.
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?
@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. |
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'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.
@acdlite We change our minds from |
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. |
Yeah that was just a mistake we made when filling out the chart. That's what my notes say. |
I think this is the only unanswered question:
|
Updates:
|
I opened an issue about the case of Symbols - #10512 |
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.)
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. |
What is the best way for me to work against this test suite? Should I send PRs to @acdlite's fork? |
@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. |
1be8c74
to
c12715e
Compare
@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. |
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. |
Oh and I'll rebase before merging to make sure you get credit for these changes :) |
Okay. I'll take a look! |
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); |
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.
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.
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.
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.
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.
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.
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.
Cool. Sounds good!
return true; | ||
} | ||
var prefix = lowerCased.slice(0, 5); | ||
return prefix === 'data-' || prefix === 'aria-'; |
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.
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.
Ah, 👍 |
expectDev(console.error.calls.argsFor(0)[0]).toContain( | ||
'Warning: Invalid prop `whatever` on <div> tag', | ||
); | ||
expect(el.getAttribute('whatever')).toBe('true'); |
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 a case we didn't test or discuss - I think this approach makes sense.
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. :) |
If boolean attributes are passed through, can we avoid stringifying them? That way we can support boolean attributes |
@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, I think we all agree that passing booleans through is not ideal. |
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. |
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 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 Right now users have to know to pass empty strings and |
@aweary's thoughts makes sense to me, but how do we move forward here? Should we agree one way or the other? |
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. |
}, | ||
}; | ||
|
||
testUnknownAttributeAssignment({hello: 'world'}, '[object Object]'); |
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.
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.
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.
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.
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 ' + |
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.
Could you set functions as properties?
Do you want to bring this up to date? |
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
I don't think we use this convention anywhere.
36e1616
to
ce03f1e
Compare
**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
Tests pass for my locally - feel free to merge if so, otherwise I'll merge on Monday. |
'If this is expected, cast the value to a string.\n' + | ||
' in div (at **)', | ||
); | ||
expectDev(console.error.calls.count()).toBe(1); |
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 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.
'If this is expected, cast the value to a string.\n' + | ||
' in div (at **)', | ||
); | ||
expectDev(console.error.calls.count()).toBe(1); |
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.
Same here.
expectDev(console.error.calls.count()).toBe(1); | ||
}); | ||
|
||
it('coerces objects to strings **and warns**', () => { |
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.
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
@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.