-
Notifications
You must be signed in to change notification settings - Fork 613
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
Encode ..'s in asset paths fixes #290 #533
base: main
Are you sure you want to change the base?
Conversation
I'm not a fan of this change. Why not just add all assets as a root or watchFolder? |
@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 react-native-community/cli#939 (comment) where this has had to be fixed in other parts of the workflow, so I think probably matching behavior is useful. |
That doesn't answer my question, why not include all those files in roots/watch folders? |
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 metro/packages/metro/src/Assets.js Line 277 in 6f8b7b8
|
Maybe to better illustrate in our project we have multiple yarn workspaces, one for a design system |
@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 |
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. |
@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 |
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? |
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. |
@imownbey Thanks much for this PR 👍 // 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 Solution here is to simply add the global regex flag to the two replace statements you introduced in this PR. localPath.replace(/\.\.\//g, '__/'); and relativePath.replace(/__(\/|\\)/g, '..$1') |
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. |
any progress on this? |
I'd love to have this too. this allow assets to be found: watchFolders: [path.resolve(__dirname, '../../../node_modules')], BUT on android, it doesn't work, because
is reduced to
even before the request is dealt with. => so it ultimately is not found |
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 ingetAsset
.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.