Encode ..'s in asset paths fixes #290

[imownbey]

Summary Android's http client will strip out '..'s from urls before sending a request. This causes projects using yarn workspaces with assets hosted outside of the project root to fail. For more information you can see #290 .

This fixes that by encoding ..'s as __'s inside getAssetData and then unencodes the __'s as ..'s in getAsset.

Other things I considered were moving path to a query param. This seems like it would require a change in React Native as well as Metro though so is maybe a bit more dangerous.

Caveat of this change is it will break anyone who has directories named __ which is probably okay, happy to change encoding to something else though.

Test plan

Updated tests to reflect the changed URLs.

[cpojer]

I'm not a fan of this change. Why not just add all assets as a root or watchFolder?

[imownbey]

@cpojer Often people like to have assets live alongside the javascript, and when you have a project broken up into yarn workspaces that can mean that assets live above the root. I agree this isn't the best solution but it seemed to be the one with the smallest footprint.

The current behavior is ..'s will be in URLs in iOS and stripped out in Android so it is definitely broken behavior even if you disagree with people using workspaces and seems like one people will continue to run into. You can see other PRs like https://github.com/react-native-community/cli/pull/939#merged-event where this has had to be fixed in other parts of the workflow, so I think probably matching behavior is useful.

[cpojer]

That doesn't answer my question, why not include all those files in roots/watch folders?

[imownbey]

I believe the asset is always referenced from the projectRoot and not from watchFolders, although would be interested in understanding how I could fix this with watchFolders https://github.com/facebook/metro/blob/6f8b7b8357ec8e5b2ac35cecb99cef7417134374/packages/metro/src/Assets.js#L277

[imownbey]

Maybe to better illustrate in our project we have multiple yarn workspaces, one for a design system packages/designSystem/src/ and one for the app packages/app/[src, ios, android, etc]. We need to make projectRoot packages/app/ so when something in say packages/designSystem/src/Icons/X.jsx does a require('./icon.png') and then is used inside of app the asset relative path is ../designSystem/src/Icons/icon.png but OKHTTP strips out those first ../ so the path sent to metro is just designSystem/src/Icons/icon.png which 404s

[janicduplessis]

@cpojer I think I pointed @imownbey towards this solution. This does happen for files included inside watchFolders (but not projectRoot). The case mentioned in the previous comment illustrates this. I guess an alternative to encoding ../ would be to have paths relative to the outer most watchFolder instead.

[cpojer]

I'm in favor of merging this if we don't use a special symbol as part of the path that may cause conflicts. That seems hacky. Let's find an elegant solution that works.

[janicduplessis]

@cpojer What about using some unique identifier like the asset hash (or whatever is the key of the asset in metro) and use that to build httpServerLocation. Maybe something like /assets/<hash>.<ext>. Then metro can extract that hash and lookup the path on disk to return the asset data.

[cpojer]

I think that could work, yeah. I'm not sure how much work it is. We'd only have to do it for assets that are outside of the root, right?

[imownbey]

Happy to look into that. Do you think the hash or moving the path to a param would be easier?

@cpojer I’d think it’d be better to request all assets the same way (by hash if moving to that) rather than changing based on if it’s in or out of the project dir, just for conformity sake. But if you think it’s better to one-off out of project files I can just do that. I just worry about bugs in one case and not the other being harder to track down.

[belemaire]

@imownbey Thanks much for this PR :+1: We are actually running into this issue, and are now hot patching metro with your fix while waiting for it to get merged, or a better solution offered (the hacky workaround suggested in https://github.com/facebook/metro/issues/290#issuecomment-543746458 won't work for us as we need to accommodate many different projects structures). Just wanted to let you know that there is actually an issue with the code in your PR having to do with the replace statements. For example :

// OKHTTP (Android HTTP client) strips out any '..' from a URL so we replace them with 2 _'s.
const escapedLocalPath = localPath.replace(/\.\.\//, '__/');

The problem here that you are mentioning that you replace any '..' from an URL with __, while the code will actually only replace the first occurence (i.e ../ will be replace by __/ but ../../../ will be replaced by __/../../). The tests are actually only testing out a single level so I guess its slipped through.

Solution here is to simply add the global regex flag to the two replace statements you introduced in this PR. i.e

localPath.replace(/\.\.\//g, '__/');

and

relativePath.replace(/__(\/|\\)/g, '..$1')

[imownbey]

Ah nice catch @belemaire. Updated test and added that. It might be good to only replace ..'s at the start of the path, happy to do that if people would prefer.

[sibelius]

any progress on this?

[rvion]

I'd love to have this too. this is blocking for me as I share assets from a node_module above project root between several apps.

this allow assets to be found:

  watchFolders: [path.resolve(__dirname, '../../../node_modules')],

BUT on android, it doesn't work, because

/assets/../../../node_modules/foo/bar

is reduced to

/node_modules/foo/bar/

even before the request is dealt with. => so it ultimately is not found

[rvion]

@imownbey @cpojer do you know a workaround for this ?