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

[asset][expo] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps #12230

Closed
wants to merge 4 commits into from

Conversation

mogery
Copy link

@mogery mogery commented Mar 17, 2021

Why

As stated in issue #11888, expo-asset (and additionally, expo's logging.fx.ts) accessed Constants.manifest on bare workflow apps without proper existance checks beforehand. This resulted in annoying yellowbox warnings in bare workflow debug builds.

Therefore, this PR fixes #11888.

How

I changed expo-assets -> PlatformUtils.getManifest to use IS_MANAGED_ENV and IS_BARE_ENV_WITH_UPDATES to determine available sources for the manifest. If IS_MANAGED_ENV is true, it will use Constants.manifest, if IS_BARE_ENV_WITH_UPDATES is true, it will use Updates.manifest, else it will just return {}. A similar method is used in expo -> environment/logging.fx.

This added expo-updates as a dependency to expo and expo-asset.

(I also had to change expo-asset -> AssetSources-test._mockConstants to set appOwnership, since that is how IS_MANAGED_ENV is determined.)

Test Plan

  • Ran yarn test on expo and expo-assets
  • Copied updated modules/builds to a bare app to confirm fix

I did not write any extra tests for this, since I don't think it's needed.

@mogery mogery requested review from ide and tsapeta as code owners March 17, 2021 09:13
@mogery
Copy link
Author

mogery commented Mar 17, 2021

I can't find how to fix this symlink error for the life of me. Can someone help?

@mogery
Copy link
Author

mogery commented Mar 17, 2021

Nevermind, this should fix it.

@mogery mogery changed the title Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps [BUG_FIXES] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps Mar 17, 2021
@mogery mogery changed the title [BUG_FIXES] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps Mar 17, 2021
@mogery
Copy link
Author

mogery commented Mar 17, 2021

Is Danger's GitHub token broken? Seems to always fail.

@mogery mogery changed the title Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps [expo-asset][expo] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps Mar 17, 2021
@mogery mogery changed the title [expo-asset][expo] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps [asset][expo] Add manifest check in expo and expo-asset to avoid yellowbox on bare workflow apps Mar 17, 2021
@esamelson
Copy link
Contributor

Thank you very much for the PR @mogery ! Many apologies that we already happened to be working on this issue at the same time, otherwise we'd gladly work with you to review and accept your PR.

In this case, we're choosing the solution in #12237 instead, for a couple of reasons -- (1) despite your comment on that PR, it does actually fall back to Updates.manifest, by utilizing the logic that already exists in Constants.ts; we'd prefer to expose an internal-facing property on Constants rather than maintain two separate copies of the same logic for finding the manifest, and (2) unfortunately adding expo-updates as a dependency to other packages comes with side effects that we don't want, so we try to avoid this whenever possible.

However, we really appreciate the time and effort you put into this PR -- I know it's not so easy to even get the repo set up currently! -- and look forward to other fixes/contributions from you in the future.

@esamelson esamelson closed this Mar 17, 2021
@mogery
Copy link
Author

mogery commented Mar 17, 2021

Ooooh, sorry, I wasn't aware that Constants had that behaviour already. No problem, my apologies.

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.

expo-asset relies on Constants.manifest and throws yellowbox warnings in bare projects
2 participants