async mediaCapabilities

[wingleung]

resolves #50

[anton-karlovskiy]

@wingleung cc @addyosmani

Thank you. I appreciate your work on this PR.
Have you got a chance to see #51 (review)?
This is not completed yet but I'd like to suggest some updates on naming and README docs.
I'm going to comment on above PR line by line to highlight potential improvements.

[anton-karlovskiy]

@wingleung cc @addyosmani

I've just finished commenting on #51 myself to highlight potential improvements.
@wingleung Would you mind checking those comments and updating this PR if you think my suggestions sound reasonable?

[wingleung]

@anton-karlovskiy I pushed your suggestion to this PR, if this gets traction and encoding seem needed we can easily add it then. I wonder if it would get used.

[anton-karlovskiy]

@wingleung cc @addyosmani

Thank you. I appreciated your working on this PR updates.
Now I don't think encoding is so important as decoding is, but I thought it could have been better if we had made the hook name look explicit.
I will review it shortly.

[anton-karlovskiy]

@wingleung

Would you mind checking my comments on this PR? Thank you!

[wingleung]

@anton-karlovskiy Also fixed the tests, they weren't blocking. And created another ticket for supporting encoding information, currently not available in chrome 👉 #54

[anton-karlovskiy]

@wingleung cc @addyosmani

Thank you @wingleung !
This PR looks good to me now. @addyosmani

For #54, I'd like to ask @addyosmani first.

[anton-karlovskiy]

@wingleung cc @addyosmani

I wonder if this raises hook warning.
I think the logic inside useEffect hook would be executed every re-rendering of the component with this hook inside. It would not be good for performance.

I think it could be something like the following.

useEffect(() => {
    supported &&
    navigator
      .mediaCapabilities
      .decodingInfo(mediaDecodingConfig)
      .then(setMediaCapabilitiesInfo)
      .catch(error => console.error(error));
  }, [supported, mediaDecodingConfig]);

[wingleung]

We can pass an empty array to have the componentDidMount effect.

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument.
https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

[anton-karlovskiy]

Thank you @wingleung for your updating.

Yes, you're correct.
As far as I know, if then, the useEffect hook would raise warnings because dependencies like supported and mediaDecodingConfig are missing.

https://stackoverflow.com/questions/55840294/how-to-fix-missing-dependency-warning-when-using-useeffect-react-hook

If we tried that hook in the real react project (dev env), then we could know it.

[anton-karlovskiy]

If possible, would you mind adding ; at the end of useEffect hook for consistency?

[anton-karlovskiy]

Thank you @wingleung

[addyosmani]

LGTM

[addyosmani]

Thanks for working on these changes, @wingleung. Overall these look solid.

There are a few minor changes we would like to make regarding useMediaCapabilitiesDecodingInfo, but @anton-karlovskiy is going to look at these shortly.

Let's get this into the release.