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.

Sign up for GitHub

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

Jump to bottom

[Web] Checking window before accessing #646

Merged
merged 9 commits into from Nov 28, 2022
Merged

[Web] Checking window before accessing #646

merged 9 commits into from Nov 28, 2022

Conversation

bombillazo
Copy link
Contributor

@bombillazo bombillazo commented Nov 28, 2022 •
edited

Overview

Solving #645

Add checks to web version of NetInfo to avoid trying to access properties of undefined. This scenario can occur when using this package with Server Side Rendering (SSR).

Test Plan

After adding these, I was able to build the Next.js app.

@bombillazo bombillazo changed the title check if window object exists in web before trying to avoid accessing… [Web] Checking window before accessing Nov 28, 2022
@bombillazo bombillazo mentioned this pull request Nov 28, 2022
Closed
mikehardy
mikehardy requested changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I think with typescript optional chaining, it can be simpler. What do you think?

src/internal/nativeModule.web.ts Outdated Show resolved Hide resolved
src/internal/nativeModule.web.ts Outdated Show resolved Hide resolved
src/internal/nativeModule.web.ts Outdated Show resolved Hide resolved
src/internal/nativeModule.web.ts Outdated Show resolved Hide resolved
src/internal/nativeModule.web.ts Outdated Show resolved Hide resolved
bombillazo and others added 6 commits November 28, 2022 09:57
@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
1ac2d25 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
9710268 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
2beed29 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
89653a3 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
55acb66 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@bombillazo
revert to explicitly check if window is not undefined
e0ab775
@bombillazo
Copy link
Contributor Author

For some reason, optional chaining is not working when Next transpiles the module, so reverted to using an expression to check for undefined

@bombillazo bombillazo requested review from mikehardy and removed request for matt-oakes and mikehardy November 28, 2022 14:06
mikehardy
mikehardy requested changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

as mentioned in direct comment - really surprising on the optional chaining failure but there is nothing like an experimental result and you have conducted the experiment. So just one tiny detail I think ?

src/internal/nativeModule.web.ts Show resolved Hide resolved
mikehardy
mikehardy approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

As mentioned - still surprising the optional chain doesn't work, but a tested result is more important than a hypothesis, and it sounds like this code works and has same return types. Fantastic. Thanks for posting this up and checking through it

@bombillazo @mikehardy
Update src/internal/nativeModule.web.ts
84a2cc9 
Co-authored-by: Mike Hardy <github@mikehardy.net>
@mikehardy mikehardy added the pending merge A PR that will be merged shortly, waiting for CI or final comment label Nov 28, 2022
@mikehardy mikehardy merged commit ba5c22c into react-native-netinfo:master Nov 28, 2022
github-actions bot pushed a commit that referenced this pull request Nov 28, 2022
@semantic-release-bot
chore(release): 9.3.7 [skip ci]
3e251ab 
## [9.3.7](v9.3.6...v9.3.7) (2022-11-28)

### Bug Fixes

* **web:** check `window` is defined before accessing  ([#646](#646)) ([ba5c22c](ba5c22c))
@matt-oakes
Copy link
Collaborator

🎉 This PR is included in version 9.3.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehardy mikehardy removed the pending merge A PR that will be merged shortly, waiting for CI or final comment label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bombillazo @matt-oakes @mikehardy