some suggestions to unify code #44
Conversation
network/index.js
Outdated
| unsupported = true; | ||
| } | ||
| const useNetworkStatus = (initialEffectiveConnectionType = null) => { | ||
| const supported = (typeof navigator !== 'undefined' && 'connection' in navigator && 'effectiveType' in navigator.connection) |
There was a problem hiding this comment.
Mentioned above.
There was a problem hiding this comment.
semicolon please.
There was a problem hiding this comment.
I think this initialization needs to be in the hook because we use useState
There was a problem hiding this comment.
hmm I get the feeling moving the supported initialization should work but it's failing some of the tests for useNetworkStatus() and I'm not sure if it's a jest/jsdom thing or a react hook/useState thing.
There was a problem hiding this comment.
Thank you for your updates.
Have you made some progress on it? I'll also try and fix it.
There was a problem hiding this comment.
As long as we've considered this conditional typeof window !== 'undefined' I think it won't throw error.
There was a problem hiding this comment.
useMemoryStatus, useHardwareConcurrency, useHardwareConcurrency
For above hooks or utilities, I think we have already moved unsupported out of the main function.
There was a problem hiding this comment.
I moved them out of the hooks for the others indeed, would be nice to move it for the network one too. The useState somehow needs te be treated differently in the tests than the ones without useState, I'll have another look at it
There was a problem hiding this comment.
@anton-karlovskiy finally got it working, the tests need to load the modules in a jest.isolateModules() function. This uses a sandboxed registry to ensure no states outside of the hook is being reused in the other tests.
There was a problem hiding this comment.
Thank you for all your efforts through getting it work.
I'll look through the changes made to learn how jest.isolateModules() has been used.
|
Overall the optimization looks good to me. @wingleung Could you please check my comments? |
|
@anton-karlovskiy since the new commits use the |
That sounds good to me. But first let's ask @addyosmani. |
wingleung
force-pushed
the
feature/code-cleanup-suggestions
branch
from
4b4731d to
cf8b7f8
Compare
|
There are conflicting files. |
wingleung
force-pushed
the
feature/code-cleanup-suggestions
branch
from
58aba42 to
ef759f5
Compare
…nup-suggestions # Conflicts: # media-capabilities/index.js # media-capabilities/media-capabilities.test.js # save-data/index.js # save-data/save-data.test.js
|
@anton-karlovskiy thanks for the heads up, merge conflicts resolved |
kicking off #37
wondering if we could change the api response to return a
supportedproperty instead of the existingunsupportedproperty. If we change it tosupportedthere is no double negatives confusion.