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

Fixed invalid prop type warning to be more specific. #11300

Closed
wants to merge 8 commits into from

Conversation

NicBonetto
Copy link
Contributor

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

  1. Fork the repository and create your branch from master.
  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. Format your code with prettier (yarn prettier).
  6. Make sure your code lints (yarn lint). Tip: yarn linc to only check changed files.
  7. Run the Flow typechecks (yarn flow).
  8. If you haven't already, complete the CLA.

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

@NicBonetto
Copy link
Contributor Author

#11289

@NicBonetto NicBonetto changed the title Fixed invalid prop type warning to be more specific. #11289 Fixed invalid prop type warning to be more specific. Oct 20, 2017
@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2017

The person might still be trying to pass it as a string though. We should probably suggest both cases.

If you intentionally tried to pass a boolean, pass it as a string instead: %s="%s". If you mean to conditionally pass an attribute, use a ternary expression: %s={condition ? value : null}.

@NicBonetto
Copy link
Contributor Author

NicBonetto commented Oct 20, 2017

Would you like me to change the test cases as well? An example would be change ReactDOMComponent-test.js line 2058 from:
Warning: Received true for non-boolean attribute whatever
to:
Warning: If you intentionally tried to pass a boolean, pass it as a string instead: true=\"whatever\". If you mean to conditionally pass an attribute, use a ternary expression: in div (at ReactDOMComponent-test.js:2053)={condition ? value : null}.

@gaearon
Copy link
Collaborator

gaearon commented Oct 20, 2017

Yes, we would need to fix the test cases. But the message you provided is wrong. When I said %s I meant that you'd need to substitute the attribute name and value there so that it looks like (for example if attribute is my-attribute is being passed true):

If you intentionally tried to pass a boolean, pass it as a string instead: my-attribute="true". If you mean to conditionally pass an attribute, use a ternary expression: my-attribute={condition ? value : null}.

If you read the message you posted, it appears completely broken. So I didn't mean that message. 😉 Please approach this from a user perspective: would a message with stack in the middle like (at ReactDOMComponent-test.js:2053)={condition ? value : null} make sense to you? Probably not. Then this is not what should be done.

gaearon and others added 6 commits October 20, 2017 20:14
* Flatten everything

* Fix ReactDOMServerNode build

* Fix native builds
While testing some changes to RN, I noticed that the '--sync-fbsource' flag had been broken recently by things being moved around and the newly-added CS renderer. Fixed it up.
* Split performWork into renderRoot and commitRoot

It turns out that the scheduler is too coupled to how the DOM renderer
works. Specifically, the requestIdleCallback model, and how roots are
committed immediately after completing. Other renderers have different
constraints for when to yield and when to commit work.

We're moving towards a model where the scheduler only works on a single
root at a time, and the render phase and commit phase are split into
distinct entry points. This gives the renderer more control over when
roots are committed, coordinating multiple roots, deferring the commit
phase, batching updates, when to yield execution, and so on.

In this initial commit, I've left the renderers alone and only changed
the scheduler. Mostly, this involved extracting logic related to
multiple roots and moving it into its own section at the bottom of the
file. The idea is that this section can be lifted pretty much as-is
into the renderers. I'll do that next.

* Remove FiberRoot scheduleAt

Isn't actually used anywhere

* Make the root schedule a linked list again

Since this still lives inside the renderer, let's just use the
FiberRoot type. The FiberRoot concept will likely be lifted out
eventually, anyway.

* commitRoot should accept a HostRoot

This way it's less reliant on the alternate model

* Unify branches

* Remove dead branch

onUncaughtError is only called while we're working on a root.

* remainingWork -> remainingExpirationTime

I was wary of leaking NoWork but mixing numbers and null is worse so
let's just do it until we think of something better.

* Rename stuff
@NicBonetto
Copy link
Contributor Author

NicBonetto commented Oct 20, 2017

Closing this pr, will reopen another with the correct code in a few minutes.

@NicBonetto NicBonetto closed this Oct 20, 2017
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.

5 participants