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

fix: downloadExpoGoAsync #22398

Merged
merged 2 commits into from
May 17, 2023
Merged

fix: downloadExpoGoAsync #22398

merged 2 commits into from
May 17, 2023

Conversation

0618
Copy link
Contributor

@0618 0618 commented May 5, 2023

Why

Fix issue #21672.

As mentioned, bar! in downloadExpoGoAsync causes error

TypeError: Cannot read properties of null (reading 'update')

How

Test Plan

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 5, 2023
@0618 0618 changed the title fix downloadExpoGoAsync fix: downloadExpoGoAsync May 5, 2023
EvanBacon
EvanBacon approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

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

update changelog please, otherwise lgtm

@0618
Copy link
Contributor Author

0618 commented May 17, 2023

update changelog please, otherwise lgtm

Thanks @EvanBacon . Changelog is added.

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 17, 2023
@EvanBacon EvanBacon merged commit 8e2f0cc into expo:main May 17, 2023
2 of 5 checks passed
@0618 0618 deleted the patch-1 branch May 18, 2023 02:17
@0618 0618 mentioned this pull request May 31, 2023
Closed
@joelpierre
Copy link

I don't believe this fixes the issue. the ! in bar! is basically telling TS to ignore that it might not be there and assume it is there anyway. using an else changes the behaviour and would now mean that bar.update isn't called if you need to re-assign "bar" right?

You should only call a method on bar if bar exists so why in this PR do we not do that specifically?

brentvatne pushed a commit that referenced this pull request May 31, 2023
@0618 @zhamujun @brentvatne
fix: downloadExpoGoAsync (#22398)
3e2ec64 
<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

Fix issue #21672.

As mentioned, `bar!` in `downloadExpoGoAsync` causes error

> TypeError: Cannot read properties of null (reading 'update')

<!--
How did you build this feature or fix this bug and why?
-->

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: MJ☔ <zhamujun@amazon.com>
@brentvatne brentvatne added the published Changes from the PR have been published to npm label May 31, 2023
@0618
Copy link
Contributor Author

0618 commented Jun 1, 2023

I don't believe this fixes the issue. the ! in bar! is basically telling TS to ignore that it might not be there and assume it is there anyway. using an else changes the behaviour and would now mean that bar.update isn't called if you need to re-assign "bar" right?

You should only call a method on bar if bar exists so why in this PR do we not do that specifically?

It actually fixed the issue. ! somehow didn't work there, which I don't know why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EvanBacon EvanBacon EvanBacon approved these changes

@byCedric byCedric Awaiting requested review from byCedric byCedric is a code owner

Assignees
No one assigned
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@0618 @joelpierre @EvanBacon @brentvatne @expo-bot @zhamujun