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: SFDX: Rename Component now shows an error message if one uses an existing file name as the new component name to avoid data loss. #4039

Merged
merged 9 commits into from
Apr 21, 2022

Conversation

floralan
Copy link
Contributor

@floralan floralan commented Apr 14, 2022

What does this PR do?

Fix the bug that SFDX: Rename Component can silently replace a file if one uses an existing file name as new component name.

What issues does this PR fix or reference?

#4024, @W-10983487@

Functionality Before

if one renames a lightning/aura component and there is an existing file with the same name, it will be replaced without warning.

Functionality After

Added an error message saying that: "the component name is already in use for a file under current component directory" to prevent user from using an existing file name as new component name to avoid file loss.

Copy link
Contributor

@sbudhirajadoc sbudhirajadoc left a comment

Choose a reason for hiding this comment

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

Change -- Component name is already in use for a file in current component directory.

To. -- This component name is already in use in the current component directory. Choose a different name and try again.

Copy link
Contributor

@gbockus-sf gbockus-sf left a comment

Choose a reason for hiding this comment

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

good stuff. some nits to consider

@floralan
Copy link
Contributor Author

good stuff. some nits to consider

Great suggestions! Have updated the code based on your comments.

@@ -154,6 +156,31 @@ async function isDuplicate(componentPath: string, newName: string): Promise<bool
return (allLwcComponents.includes(newName) || allAuraComponents.includes(newName));
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Good use of the "why" in a comment 👍

Copy link
Contributor

@randi274 randi274 left a comment

Choose a reason for hiding this comment

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

QA Complete

  • Can rename LWC component without an duplicate names → ✅
  • Can rename LWC component w/ another component having a JS file with the intended name → ✅
  • Can’t rename LWC component with another LWC component bundle having the intended name → ✅
  • Can’t rename LWC component w/ Aura having the intended bundle name → ✅
  • Can’t rename Aura component w/ LWC having the intended bundle name → ✅
  • Can’t rename LWC component that has a .js file (or any other file) matching the new intended name ✅
  • Can’t rename LWC component that has a .js file (or any other file) matching the new intended name ✅
  • Can’t run the command from the command palette at all ✅

I did a little extra QA since I didn't get a chance to on initial pass through. I also encountered a few additional questions about the feature, neither of which should prevent merge of this fix.

  • ❓I noticed SFDX: Rename Component was not available on the tests directory or any files therein. Should it be?
  • ❓Is it possible to reopen the new component after renaming? When I run the default Rename it does reopen the bundle, and currently functionality is that focus is lost and I have to find the component again.

@floralan
Copy link
Contributor Author

QA Complete

  • Can rename LWC component without an duplicate names → ✅
  • Can rename LWC component w/ another component having a JS file with the intended name → ✅
  • Can’t rename LWC component with another LWC component bundle having the intended name → ✅
  • Can’t rename LWC component w/ Aura having the intended bundle name → ✅
  • Can’t rename Aura component w/ LWC having the intended bundle name → ✅
  • Can’t rename LWC component that has a .js file (or any other file) matching the new intended name ✅
  • Can’t rename LWC component that has a .js file (or any other file) matching the new intended name ✅
  • Can’t run the command from the command palette at all ✅

I did a little extra QA since I didn't get a chance to on initial pass through. I also encountered a few additional questions about the feature, neither of which should prevent merge of this fix.

  • ❓I noticed SFDX: Rename Component was not available on the tests directory or any files therein. Should it be?
  • ❓Is it possible to reopen the new component after renaming? When I run the default Rename it does reopen the bundle, and currently functionality is that focus is lost and I have to find the component again.

@randi274 Thank you for the very detailed QA report! For your first questions, I changed the code to enable rename command for tests folder and any .js files under it. Could you take time to verify you can see the change on your end as well? For the second question, I think that's a very good feature users may expect. We can create a user story for it!

@randi274
Copy link
Contributor

Thanks for making that change so quickly @floralan! I think it might be a bit more of a complicated story, as it doesn't look like it's working as I'd expect quite yet; can you revert that commit, and then we'll add another user story to address that one?

@floralan floralan force-pushed the fl/rename_deduplicate_file_name branch from 5e0cfad to 1b6004a Compare April 21, 2022 16:51
@floralan
Copy link
Contributor Author

floralan commented Apr 21, 2022

@randi274 Yes, making another story for the changes sounds a better plan! I just reset the branch, once all the tests done, we can get this PR merged 👍

@floralan floralan merged commit fe7ec6a into develop Apr 21, 2022
@floralan floralan deleted the fl/rename_deduplicate_file_name branch April 21, 2022 17:27
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.

None yet

4 participants