search

react-native-community / upgrade-helper

⚛️ A web tool to support React Native developers in upgrading their apps.
https://react-native-community.github.io/upgrade-helper
MIT License
3.67k stars 112 forks source link

Fix file path when viewing or downloading file #209

Closed benomatis closed 4 years ago

benomatis commented 4 years ago

Related to https://github.com/react-native-community/upgrade-helper/pull/206, I unfortunately used an incorrect Regex, it's fixed here. Apologies.

lucasbento commented 4 years ago

Hey @benomatis, thank you for fixing this.

There's however still an issue, if you go to https://deploy-preview-209--upgrade-helper.netlify.com/?from=0.61.5&to=0.62.0-rc.5, then change the name of the app to something like awesomeapp, look for a file like android/app/src/debug/java/com/awesomeapp/ReactNativeFlipper.java and then tap View File you will see that it goes 404.

That's because it should lead to android/app/src/debug/java/com/rndiffapp/ReactNativeFlipper.java instead.

Would you have time to look into that?

benomatis commented 4 years ago

Hey @benomatis, thank you for fixing this.

There's however still an issue, if you go to https://deploy-preview-209--upgrade-helper.netlify.com/?from=0.61.5&to=0.62.0-rc.5, then change the name of the app to something like awesomeapp, look for a file like android/app/src/debug/java/com/awesomeapp/ReactNativeFlipper.java and then tap View File you will see that it goes 404.

That's because it should lead to android/app/src/debug/java/com/rndiffapp/ReactNativeFlipper.java instead.

Would you have time to look into that?

I guess the best approach would be to store the original paths, even upon app name change, and use them for view and download; the regex could become quite complex if we wanted to account for these.

lucasbento commented 4 years ago

I guess the best approach would be to store the original paths, even upon app name change, and use them for view and download; the regex could become quite complex if we wanted to account for these.

Yeah, I completely agree with this.

benomatis commented 4 years ago

@lucasbento It took me a while because the appName is changed in DiffViewer from the diff before it being parsed, but it can only be passed on to DiffSection after having been parsed, so I ended up parallelly parsing the diff but not applying the replaceAppName on it, and using the index of the diff with unchanged app name to get its newPath and supply it to the originalPath variable. I hope this is not to complex or resource hungry, I don't see a performance degradation, the way I implemented it, but feel free to simplify, of course.

lucasbento commented 4 years ago

Hey @benomatis, thanks for changing it.

I'm gonna take a better look at the diff later today, perhaps we don't need to parse the diff twice to get it fixed, I'm a little bit worried about how much the processing is increased by doig that.

benomatis commented 4 years ago

Hey @benomatis, thanks for changing it.

I'm gonna take a better look at the diff later today, perhaps we don't need to parse the diff twice to get it fixed, I'm a little bit worried about how much the processing is increased by doig that.

Sure, please do, I still have lots to learn ;)

benomatis commented 4 years ago

Happy for this to be closed for https://github.com/react-native-community/upgrade-helper/pull/214