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

"rename file" and "move to new file" refactors do not work when using files #24857

Closed
OliverJAsh opened this issue Jun 11, 2018 · 40 comments · Fixed by #25623
Closed

"rename file" and "move to new file" refactors do not work when using files #24857

OliverJAsh opened this issue Jun 11, 2018 · 40 comments · Fixed by #25623
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Needs More Info The issue still hasn't been fully clarified

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Jun 11, 2018

TypeScript Version: 2.9.1

Search Terms:
files "move to new file" "rename file"

Code

// tsconfig.json
{
  "files": ["./src/index.ts"]
}
// ./src/index.ts
import foo from './other';
// ./src/other.ts
export default {}

const bar = {};

Expected behavior:

  • When renaming the file ./src/other.ts, I expect TypeScript/VSCode to prompt me to update the import in ./src/index.ts.
  • When selecting bar in ./src/other.ts, TypeScript/VSCode offers to move this to a new file. When actioning this, I expect this to work.

(TypeScript knows that ./src/other.ts is in the project because it is part of the traced dependency graph from the entry file ./src/index.ts.)

Actual behavior:

  • When renaming the file ./src/other.ts, there is no prompt to update the import.
  • When selecting bar in ./src/other.ts, TypeScript/VSCode offers to move this to a new file. When actioning this, nothing happens.

Playground Link:

Related Issues:
microsoft/vscode#51366
#23573
#23726


A known workaround is to use the include option instead of files. However, I am curious why this is necessary. I prefer to list the entry file and let TypeScript work the rest out. This is especially helpful when a folder has a combination of source and test files—TypeScript automatically knows not to include the test files because they are not part of the dependency graph.

If there is a good reason why these commands/refactors can't work with files, could we make the experience clearer? Why is the "move to new file" command suggested in VSCode when it leads to nothing? If so, perhaps this is an issue for VSCode.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 11, 2018

Can you share the log from your tsserver? i suspect it is a duplicate of #24613

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Jun 11, 2018
@OliverJAsh
Copy link
Contributor Author

@mhegazy Does this help? tsserver.log

@mhegazy mhegazy added Bug A bug in TypeScript and removed Needs More Info The issue still hasn't been fully clarified labels Jun 12, 2018
@mhegazy mhegazy added this to the TypeScript 3.0 milestone Jun 12, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jun 12, 2018

This looks different.

@sheetalkamat
Copy link
Member

@mjbvz this seems like there are 2 open requests on same file. Can you check why? We do not expect open on same file without closing it first. Thats the assert that raised error in the log.
Interesting open requests:

Info 7    [11:4:3.833] request:
    {"seq":0,"type":"request","command":"open","arguments":{"file":"/Users/OliverJAsh/Development/temp/typescript-files-refactors-test/src/index.ts","fileContent":"// ./src/index.ts\nimport foo from './other';"}}

nfo 52   [11:4:7.163] request:
    {"seq":6,"type":"request","command":"open","arguments":{"file":"/Users/OliverJAsh/Development/temp/typescript-files-refactors-test/src/index.ts","fileContent":"// ./src/index.ts\nimport foo from './other';","scriptKindName":"TS","projectRootPath":"/Users/OliverJAsh/Development/temp/typescript-files-refactors-test"}}

@mjbvz
Copy link
Contributor

mjbvz commented Jun 27, 2018

@OliverJAsh Can you please try collecting the TS Server log again in the latest VS Code insiders build. I made some changes to how we track opened files and haven't been able to reproduce the duplicate open requests in recent VS Code builds

@OliverJAsh
Copy link
Contributor Author

@mjbvz In this log I attempt to use the "move to new file" command as described in my original post.
tsserver.log

@mjbvz Are you able to reproduce the issue as I described?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 30, 2018

@OliverJAsh looks like this log is using typescript@2.9.1 can you try with typescript@next?

@sheetalkamat
Copy link
Member

@OliverJAsh The error you are seeing in the log, i think is fixed by #24613. Please try typescript@next and let us know if you still see the issue. Thank you.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jul 2, 2018
@mhegazy mhegazy closed this as completed Jul 2, 2018
@OliverJAsh
Copy link
Contributor Author

I can still reproduce this issue using Code Insiders with @next. Attached logs.
tsserver.log

@mhegazy mhegazy reopened this Jul 2, 2018
@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label Jul 2, 2018
@sheetalkamat
Copy link
Member

@OliverJAsh I am not sure what the issue is here. I am seeing same behavior irrespective of tsconfig containing files entry. That is when i rename file other to other2 there is no prompt to update the imports.
Selecting and moving const bar = {}; to new file does create bar.ts and if files entry is present it adds to it. (is that the issue?) (there is no import update required here since bar is not exported.
Adding @andy-ms since he has worked on this feature and may know more.

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Jul 3, 2018

Selecting and moving const bar = {}; to new file does create bar.ts and if files entry is present it adds to it.

This doesn't match the behaviour I'm seeing. When I create to move it to a new file, nothing happens. I've attached a TS server log in which I try to do this. tsserver.log

@sheetalkamat
Copy link
Member

@andy-ms Can you please take a look. The tsserver log shows assert:

Err 265   [9:10:53.930] Exception on executing command {"seq":41,"type":"request","command":"getEditsForRefactor","arguments":{"file":"/Users/OliverJAsh/Development/typescript-playground/src/other.ts","startLine":4,"startOffset":1,"endLine":4,"endOffset":16,"refactor":"Move to a new file","action":"Move to a new file"}}:

    Debug Failure. False expression.

    Error: Debug Failure. False expression.
    at mapTextChangesToCodeEdits (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118361:22)
    at IOSession.Session.mapTextChangeToCodeEdit (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118197:24)
    at /Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118192:73
    at Array.map (native)
    at IOSession.Session.mapTextChangesToCodeEdits (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118192:36)
    at IOSession.Session.getEditsForRefactor (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118092:112)
    at Session.handlers.ts.createMapFromTemplate._a.(anonymous function) (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:116962:61)
    at /Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118299:88
    at IOSession.Session.executeWithRequestId (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118290:28)
    at IOSession.Session.executeCommand (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118299:33)
    at IOSession.Session.onMessage (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:118319:35)
    at Interface.<anonymous> (/Users/OliverJAsh/Development/typescript-playground/node_modules/typescript/lib/tsserver.js:119574:27)
    at emitOne (events.js:96:13)
    at Interface.emit (events.js:191:7)
    at Interface._onLine (readline.js:241:10)
    at Interface._normalWrite (readline.js:384:12)
    at Socket.ondata (readline.js:101:10)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:191:7)
    at readableAddChunk (_stream_readable.js:178:18)
    at Socket.Readable.push (_stream_readable.js:136:10)
    at Pipe.onread (net.js:560:20)

@sheetalkamat sheetalkamat assigned ghost Jul 3, 2018
@ghost ghost mentioned this issue Jul 3, 2018
@ghost
Copy link

ghost commented Jul 3, 2018

I can't reproduce the assert, but I have noticed that renaming other.ts sometimes produces no change, probably because it is in its own project. We need to aggregate results from all projects like in #25283.

@OliverJAsh
Copy link
Contributor Author

We need to aggregate results from all projects like in #25283.

Unrelated, but will this address #20185?

@mhegazy
Copy link
Contributor

mhegazy commented Jul 3, 2018

no.

@ghost
Copy link

ghost commented Jul 11, 2018

@OliverJAsh Could you test again now that #25522 is in?

@OliverJAsh
Copy link
Contributor Author

Is that with typescript@next?

Yes. Just tried with 3.1.0-dev.20180801.

What operating system are you on?

macOS 10.13.5.

build locally and test against that

I'm not completely familiar with how to do that. Is there a guide somewhere I can follow?

@andy-ms Are you able to reproduce the problem on your end?

@rosieks
Copy link

rosieks commented Sep 5, 2018

I'm trying to use move to new file in JavaScript but it doesn't work (no proposition to move function to another file). Should it work in JavaScript or only in TypeScript? I have created new projected using React +Redux template from ASP.NET Core 2.1

@ghost
Copy link

ghost commented Sep 5, 2018

@rosieks Please make a new issue. It should work in JavaScript. Be sure to test with typescript@next.

@rosieks
Copy link

rosieks commented Sep 5, 2018

Ok - I will create separate issue - could you just tell me where should I have typescript@next and my projects doesn't have typescript at all - I'm using VS 15.8.1

@minestarks
Copy link
Member

@rosieks move to file isn't yet supported in Visual Studio 15.8, so unfortunately even a new build of TypeScript won't help you in this case. We're working on adding VS support for this soon.

@sheetalkamat
Copy link
Member

@OliverJAsh Sorry for getting back but can you please try your repro with typescript@next and see if it repros. Providing log if it repros will help. Thanks.

@sheetalkamat sheetalkamat added the Needs More Info The issue still hasn't been fully clarified label Jan 3, 2019
@OliverJAsh
Copy link
Contributor Author

Testing "move to new file", unfortunately that doesn't seem to help.

tsserver.log

@sheetalkamat
Copy link
Member

@OliverJAsh Thank you for the log. Will investigate.

@sheetalkamat
Copy link
Member

@OliverJAsh its been now fixed by #26280 Please verify with typescript@next and report back with tsserver log if it still repros. Thank you,

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Jan 13, 2019

It fixes the problem—thank you! One small thing though: it adds the new file to files even though it doesn't need to be (when it's included via the dependency graph of existing files).

@sheetalkamat
Copy link
Member

@OliverJAsh can you please share tsserver log where your newFile gets added to the files list? I don't see that when I do it with typescript@next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants