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

fix(asset): URL encode asset paths defined as query parameter #24562

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

byCedric
Copy link
Member

Why

Package folders may contain a + symbol when using isolated modules by supported package managers. This happens for @expo/vector-icons and pnpm, for example:

image

While the path is valid, the + is interpreted as a special character (space) because it's not URL encoded.

How

  • Escape asset path (in asset.httpServerLocation) when defined as ?export_pathor?unstable_path` query parameter.

Test Plan

Checklist

@byCedric byCedric force-pushed the @bycedric/asset/escape-path-when-hashing-assets branch from 21cfe5c to 3668954 Compare September 21, 2023 15:42
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 21, 2023
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 21, 2023
byCedric added a commit to byCedric/expo-50-pnpm that referenced this pull request Sep 21, 2023
const assetPathQueryParameter = asset.httpServerLocation.match(
/\?(export_path|unstable_path)=(.*)/
);
if (assetPathQueryParameter && assetPathQueryParameter[2]) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this could look a bit better with 👇

Suggested change
if (assetPathQueryParameter && assetPathQueryParameter[2]) {
if (assetPathQueryParameter?.[2]) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure if we could use that since this file is NOT being transpiled. It's just a plain JS file being shipped as is, so I also avoided constructor magic 🙈

@byCedric byCedric merged commit 30a2771 into main Sep 22, 2023
8 checks passed
@byCedric byCedric deleted the @bycedric/asset/escape-path-when-hashing-assets branch September 22, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants