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

handle null value return from content prop #224

Merged
merged 2 commits into from
Mar 8, 2020
Merged

handle null value return from content prop #224

merged 2 commits into from
Mar 8, 2020

Conversation

a-sync
Copy link
Contributor

@a-sync a-sync commented Mar 5, 2020

  • allows a ref with null initial value in ts strict mode

 * allows a ref with null initial value in ts strict mode
Copy link
Owner

@MatthewHerbst MatthewHerbst left a comment

Choose a reason for hiding this comment

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

Thanks for making this change. It looks good overall, though the need for it is unfortunate. In an ideal world you would have the trigger disabled until your content was ready. While I'm ok solving this here, this really should be solved by the calling code.

src/index.tsx Outdated Show resolved Hide resolved
@a-sync
Copy link
Contributor Author

a-sync commented Mar 6, 2020

Thanks for making this change. It looks good overall, though the need for it is unfortunate. In an ideal world you would have the trigger disabled until your content was ready. While I'm ok solving this here, this really should be solved by the calling code.

Yes, you are correct, but unfortunetly it can't be done properly in ts with the current API.
One solution could be to use another prop which is expected to return a Ref, and only check the actual component it points to during runtime like it does now. (but this not only adds to the API, but changes the current prop to be optional (which might be a non API breaking change))

I think this error message could be improved a little, since with this change it's possible to actually just pass null content, even one that never becomes not null. Could we change this to:

Tbh. i'm not completely happy with the current solution myself, but it works for most intents and purposes.

  1. the two checks on contentEl basically do the same, which is to make sure that the returned value from content() is what we need. might aswell check for !contentEl and unify the error message (if someone tries it using useRef() instead of useRef(null) to get the ref, the wrong error message would be returned)
  2. since we already check for it, undefined might aswell be allowed as return value for the content prop (undefined is what useRef() uses anyway as the default initial value, and the current implementation only allows null with no additional benefit to this restriction imo)

@MatthewHerbst
Copy link
Owner

Tbh. i'm not completely happy with the current solution myself, but it works for most intents and purposes.

  1. the two checks on contentEl basically do the same, which is to make sure that the returned value from content() is what we need. might aswell check for !contentEl and unify the error message (if someone tries it using useRef() instead of useRef(null) to get the ref, the wrong error message would be returned)
  2. since we already check for it, undefined might aswell be allowed as return value for the content prop (undefined is what useRef() uses anyway as the default initial value, and the current implementation only allows null with no additional benefit to this restriction imo)

I hear you. The issue is that we are trying to detect people using functional components without useRef and give them a proper error message (that using it that was isn't supported). There's probably a better way to do that. I'll merge this for now and we can revisit it as needed. If you think there's a better overall solution I'm open to any discussions/PRs

@MatthewHerbst MatthewHerbst merged commit 0ed5b7e into MatthewHerbst:master Mar 8, 2020
@MatthewHerbst
Copy link
Owner

Published in v2.6.2

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

Successfully merging this pull request may close these issues.

2 participants