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

[expo-assets] search for key without file extension #12624

Merged
merged 8 commits into from
Apr 21, 2021

Conversation

jkhales
Copy link
Contributor

@jkhales jkhales commented Apr 20, 2021

Why

We are going to be dropping extensions for asset filenames for EAS updates.

How

Search for the extension-less filename first, if it doesn't exist fall back to the legacy method.

Legacy updates will continue to use the old asset scheme.

Test Plan

  • Tested with EAS updates while a local server returned assets whose keys were just the hash (so it is now conformant with the update protocol) and the client was modified to save the assets to disk without an extension.

Confirmed images loaded both by an embedded bundle and a remotely loaded bundle.

  • Tested with legacy updates and confirmed images loaded both with embedded and remotely loaded bundles.

@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against 1002249

@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against bae7800

1 similar comment
@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against bae7800

@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against 4f40db6

@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against e8eea37

@expo-bot
Copy link
Collaborator

Fails
🚫

📋 Missing Changelog

🛠 Add missing entries to:

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/expo-asset/CHANGELOG.md b/packages/expo-asset/CHANGELOG.md
index 5375be0c..9c959510 100644
--- a/packages/expo-asset/CHANGELOG.md
+++ b/packages/expo-asset/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- search for key without file extension. ([#12624](https://github.com/expo/expo/pull/12624) by [@jkhales](https://github.com/jkhales))
+
 ## 8.3.1 — 2021-03-23
 
 ### 🐛 Bug fixes

Generated by 🚫 dangerJS against 832af0d

@jkhales jkhales requested a review from esamelson April 21, 2021 00:50
@jkhales jkhales marked this pull request as ready for review April 21, 2021 00:50
@jkhales jkhales requested a review from tsapeta as a code owner April 21, 2021 00:50
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

🎉

packages/expo-asset/src/LocalAssets.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,40 @@
import LocalAssets from 'expo-constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import LocalAssets from 'expo-constants';
import Constants from 'expo-constants';

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 indeed. 😅

packages/expo-asset/src/LocalAssets.ts Outdated Show resolved Hide resolved
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

👍

@kubanac95
Copy link

Not sure why, but once this PR was merged I can no longer run the app due to following error

Unable to resolve module ./EmbeddedAssets from ..\node_modules\expo-asset\build\Asset.js

@jkhales
Copy link
Contributor Author

jkhales commented May 13, 2021

Not sure why, but once this PR was merged I can no longer run the app due to following error

Unable to resolve module ./EmbeddedAssets from ..\node_modules\expo-asset\build\Asset.js

Can you provide a bit more information?

Ideally a reproducible demo.

@kubanac95
Copy link

@jkhales I will try to make a minimalistic reproducible demo, will get back to you

@qcho
Copy link

qcho commented May 28, 2021

@kubanac95, @jkhales. Could you reproduce this error? I'm getting the same error on an bare project with "@unimodules/core": "7.1.0", dependency.

error: Error: Unable to resolve module ./EmbeddedAssets from <...>/app/node_modules/expo-asset/build/Asset.js:

None of these files exist:
  * node_modules/expo-asset/build/EmbeddedAssets(.native|.android.js|.native.js|.js|.android.json|.native.json|.json|.android.ts|.native.ts|.ts|.android.tsx|.native.tsx|.tsx)
  * node_modules/expo-asset/build/EmbeddedAssets/index(.native|.android.js|.native.js|.js|.android.json|.native.json|.json|.android.ts|.native.ts|.ts|.android.tsx|.native.tsx|.tsx)
> 1 | import { Platform } from '@unimodules/core';
  2 | import { getAssetByID } from './AssetRegistry';
  3 | import * as AssetSources from './AssetSources';
  4 | import * as AssetUris from './AssetUris';
    at ModuleResolver.resolveDependency (<...>/app/node_modules/metro/src/node-haste/DependencyGraph/ModuleResolution.js:211:15)
    at DependencyGraph.resolveDependency (<...>/app/node_modules/metro/src/node-haste/DependencyGraph.js:413:43)
    at Object.resolve (<...>/app/node_modules/metro/src/lib/transformHelpers.js:317:42)
    at resolve (<...>/app/node_modules/metro/src/DeltaBundler/traverseDependencies.js:629:33)
    at <...>/app/node_modules/metro/src/DeltaBundler/traverseDependencies.js:645:26
    at Array.reduce (<anonymous>)
    at resolveDependencies (<...>/app/node_modules/metro/src/DeltaBundler/traverseDependencies.js:644:33)
    at <...>/app/node_modules/metro/src/DeltaBundler/traverseDependencies.js:329:33
    at Generator.next (<anonymous>)
    at asyncGeneratorStep (<...>/app/node_modules/metro/src/DeltaBundler/traverseDependencies.js:137:24)

@brentvatne
Copy link
Member

@qcho - try clearing your metro cache

@kubanac95
Copy link

@qcho I could not reproduce it in a blank project. I ended up downgrading the versio on the main project since clening up cache nor node_modules helped.

@brentvatne
Copy link
Member

brentvatne commented May 28, 2021

that's odd because the error message suggests that expo-asset/build/Asset.js is attempting to import ./EmbeddedAssets, which doesn't exist -- and the only time ./EmbeddedAssets would not exist would be on a version of expo-asset where it was removed entirely. so, the most likely case here is some issue with caching. you could also open expo-asset/build/Asset.js and verify whether it tries to actually import ./EmbeddedAssets or not. i would suspect that it does not given that even the stack trace is pointing to the incorrect line. you might want to try this out: https://github.com/pmadruga/react-native-clean-project

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.

7 participants