Skip to content

Fix package name and app name replacement - #376

Open
M7mdisk wants to merge 1 commit into
react-native-community:masterfrom
M7mdisk:fix-appname-replacement
Open

Fix package name and app name replacement#376
M7mdisk wants to merge 1 commit into
react-native-community:masterfrom
M7mdisk:fix-appname-replacement

Conversation

@M7mdisk

@M7mdisk M7mdisk commented Feb 20, 2024

Copy link
Copy Markdown

Summary

the app name and package name were not being properly changed inside the diff files, only in the diff path, this is happening because the tokenizer is using the old hunks instead of updatedHunks. I also added getHunksWithAppName as a dependency to make sure the files are automatically updated when the app name or package name are changed. This isn't introducing any new features, simply fixing what appears to be a bug.

Test Plan

Before:

image

After:

image

What are the steps to reproduce?

Checklist

  • I tested this thoroughly
  • I added the documentation in README.md (if needed)

@kelset kelset left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this PR — and sorry it took us such a long time to come back around to it.

I took another pass through it as part of a maintenance review of the open PRs, and the underlying bug still looks real from what I can see: path replacement and content replacement are not fully in sync yet in some Android diffs, so I do think this is still pointing at a valid issue.

That said, I don’t think we can merge this branch as-is anymore because it’s old and the surrounding code has changed since then. In particular, this part of the diff rendering flow has had other fixes land since this PR was opened, including #378, so I’d want to make sure we don’t accidentally reintroduce any of those older rendering problems.

If you’re still interested in updating this PR, could you please:

  • rebase/update the branch on current master
  • verify the fix still works against the current Diff.tsx implementation
  • make sure it doesn’t regress the duplicated-hunks/rendering issue that was fixed later in #378
  • add/update a regression test if practical

If you’re up for refreshing it, I’d be happy to take another look after that. And if not, no worries at all — we can always revisit with a fresh PR from current master later.

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.

3 participants