JavaScript Refactoring - Rename Identifier, Wrap selection, Convert to arrow function, Create Getters & Setters by navch · Pull Request #13965 · adobe/brackets

navch · 2017-12-07T20:57:53Z

This PR implements "rename" functionality in JS mode to enable intelligent rename feature by using Tern's scope analysis and inference functions. Renaming across multiple files has been intentionally blocked for the time being as we are not analyzing all the JS files in current project root right now.

The rename function works by selecting a variable def/ref or placing cursor in and using Ctrl+R.

This code is written by @swmitra and already reviewed by @petetnt (PR 13047)

I just restructured this code and created new extension called JavascriptCodeRefactoring.

Known issues:-

In a particular case Tern inference engine is picking up wrong hit. For example :-

function fn1(error) {
    error.a = "abc";
    var a = "test";
    console.log(a);
}

If we try to rename a in line 3 or 4 , Tern picks up fake ref from line 2 as well.

#Issue reported in tern 950, 951
951 is already fixed.

Wrap Selection in Try Catch/Condition

Select code/Just put cursor anywhere
Edit -> wrap in try catch/condition Or find this in submenu
If you just placed cursor it will find surrounding statements around cursor position
else if you selected code then it will check weather these are statements or not
If statements -> It will wrap that code in TryCatch/Condition
else error (Please select valid statements)

AppInit.appReady(function () { brackets.test.doneLoading = true; });

Select whole code or just place cursor on 3rd line it will wrap whole code in try catch block/Condition block

try { AppInit.appReady(function () { brackets.test.doneLoading = true; }); } catch (e) { console.log(e.message); }
Put selection on 2nd line and do wrap in try catch/condition it will wrap that line only.

Know Issue:- Some time indention goes wrong in case of wrap in condition

Convert to arrow function

Put cursor anywhere in anonymous function and click convert to arrow function
If one param structure will be param => {statements}
If zero or more than one param (param1, param2) => {statements}
In case of one statement, it will remove "{, }" and ";"as well

Create Setters and Getters for given property

Put cursor on property
Create getter/setter
get getTest() { return this.test; }, set setTest(val) { this.test = val; }

Please review @nethip @boopeshmahendran @petetnt @swmitra @ficristo @saurabh95

…ode domain, popping up error at cursor

nethip · 2017-12-14T08:04:51Z

👍 @navch

nethip · 2017-12-14T08:05:20Z

Is it OK to squash all of your commits into one single commit?

nethip · 2017-12-18T04:38:15Z

+    var templates = JSON.parse(require("text!Templates.json"));
+
+    /**
+     * Note - Don't use this if you are in a batch operation and you already made some changes


This entire comment section needs to be rewritten.

nethip · 2017-12-18T04:47:26Z

+            offset, handleFindRefs;
+
+        if (editor.getSelections().length > 1) {
+            editor.displayErrorMessageAtCursor("Rename doesn't work in case of multicursor");


This string needs to be externalized. And correct the wording as well.

nethip · 2017-12-18T04:47:44Z

+
+    //Do rename of identifier which is at cursor
+    function handleRename() {
+        var editor = EditorManager.getActiveEditor(),


Are you going to allow rename inside quick edit session?

Yes. Rename is working great in quick edit as well for local variables. Renaming function calls could be a problem. According to me we should allow rename in quick edit as well. Please let me know if want me to disable rename in quick edit.

OK. I think it is better to disable rename for quick edit. There are chances that once you invoke a quick edit on a function name, we only show the contents of the function. But there may be variables whose scope is outside the function and I am not sure how you are going to show the feedback for renaming variable references which are not shown inside quick edit.

Let us do this. I will merge this PR. But please raise another PR addressing this concern.

Sure @nethip I will do that. Thanks for suggestion.

nethip · 2017-12-18T04:50:25Z

+        var funcExprNode = current.findSurroundASTNode(current.ast, {start: current.startIndex}, ["FunctionExpression"]);
+
+        if (!funcExprNode || funcExprNode.type !== "FunctionExpression" || funcExprNode.id) {
+            current.editor.displayErrorMessageAtCursor("Cursor is not inside function expression");


String needs to be externalized.

nethip · 2017-12-18T04:54:02Z

@navch All error popup strings need to go into Strings.js file. Please make this change. Also consider re-wording those strings.

navch · 2017-12-19T08:13:17Z

Thanks @nethip for reviewing this. I addressed all review comments.

petetnt

Few review comments

petetnt · 2017-12-19T08:26:40Z

+ * file for cursor
+ */
+ function getRefs(fileInfo, offset) {
+    var request = buildRequest(fileInfo, "refs", offset);


Can fileInfo be null or undefined ever?

This should not be the case. Anyways I handled this when we are creating fileInfo.

petetnt · 2017-12-19T08:27:18Z

+    /**
+     * This will add template at given position/selection
+     *
+     * @param {string} template - name of templated defined in templates.json


Nit: name of the template

petetnt · 2017-12-19T08:27:40Z

+     *
+     * @param {string} template - name of templated defined in templates.json
+     * @param {Array} args- Check all arguments that exist in defined templated pass all that args as array
+     * @param {{line: number, ch: number}} rangeToReplace - Range which we want to rreplace


Nit: replace

petetnt · 2017-12-19T09:31:50Z

+    //Post message to tern node domain that will request tern server to find refs
+    function getRefs(fileInfo, offset) {
+        ScopeManager.postMessage({
+            type: "getRefs",


type: MessageIds.TERN_REFS, maybe

petetnt · 2017-12-19T10:17:09Z

+         * If yes then select all references
+         */
+        handleFindRefs = function (refsResp) {
+            function isInSameFile(obj) {


isInSameFile could be externalized from the function

petetnt · 2017-12-19T10:19:10Z

+         * Check if references are in this file only
+         * If yes then select all references
+         */
+        handleFindRefs = function (refsResp) {


Maybe cleaner if this one was assigned inline above requestFindReferences?

function handleFindRefs(refsResp)...

petetnt · 2017-12-19T10:20:04Z

+        function requestFindReferences(session, offset) {
+            var response = requestFindRefs(session, session.editor.document, offset);
+
+            if (response || response.hasOwnProperty("promise")) {


This should probably be if (response && response.hasOwnProperty("promise")?

petetnt · 2017-12-19T10:27:23Z

+    "CMD_REFACTORING_CONDITION"                 : "Wrap in Condition",
+    "CMD_REFACTORING_GETTERS_SETTERS"           : "Create Getters Setters",
+    "CMD_REFACTORING_ARROW_FUNCTION"            : "Convert to Arrow Function",
+    "DESCRIPTION_CODE_REFACTORIG"               : "Enable/disable JavaScript Code Refactoring",


DESCRIPTION_CODE_REFACTORING

petetnt · 2017-12-19T10:27:41Z

+
+    // This preference controls whether to create a session and process all JS files or not.
+    PreferencesManager.definePreference("refactoring.JSRefactoring", "boolean", true, {
+        description: Strings.DESCRIPTION_CODE_REFACTORIG


DESCRIPTION_CODE_REFACTORING

…nor change in templates.json

navch · 2017-12-19T15:51:39Z

Thanks @petetnt for reviewing this. I addressed all review comments.

Naveen Choudhary added 7 commits December 8, 2017 01:35

Rename references using tern

915c0e6

On doc change editor was not getting updated

c2b7b4d

Addressing review comments

0d9cb84

First cut of wrap selection

f485ec1

Fixing issues i.e. Rename global variable, passing commonJS to tern n…

b2e1d5b

…ode domain, popping up error at cursor

Wrap in condition

2cf2cc7

Code clean up

756fb9b

Naveen Choudhary added 4 commits December 16, 2017 21:04

Wrap selction, create getters setters, arrow function

5ebc31e

Lint error fixed

8bd69f2

linitng errors fixed

ce63ccc

Final changes, added documentation

a663474

navch changed the base branch from navc/RestructuringJSCodeHints to master December 17, 2017 16:43

navch changed the title ~~Rename references using tern~~ JavaScript Refactoring - Rename Identifier, Wrap selection, Convert to arrow function, Create Getters & Setters Dec 17, 2017

nethip reviewed Dec 18, 2017

View reviewed changes

Naveen Choudhary added 2 commits December 18, 2017 14:46

Addressing review comments

b09d1dc

Removing get and set from getters and setter name

d3e7df3

petetnt reviewed Dec 19, 2017

View reviewed changes

Naveen Choudhary added 3 commits December 19, 2017 20:57

Addressed review comments, Fixed bug in convert to arrow function, mi…

d8e0060

…nor change in templates.json

linting errors fixed

3488060

linting errors fixed

abd80b9

nethip merged commit b39e89a into master Dec 20, 2017

petetnt deleted the navc/RenameIdentifier branch December 20, 2017 16:44

navch mentioned this pull request Dec 21, 2017

Don't allow rename in inline widget if that identifier have reference… #13994

Merged

Conversation

navch commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nethip commented Dec 14, 2017

Uh oh!

nethip commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nethip commented Dec 18, 2017

Uh oh!

navch commented Dec 19, 2017

Uh oh!

petetnt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navch commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

navch commented Dec 7, 2017 •

edited

Loading