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

Update import on file rename #2651

Merged
merged 19 commits into from
Mar 2, 2021
Merged

Conversation

jasonlyu123
Copy link
Contributor

@jasonlyu123 jasonlyu123 commented Jan 27, 2021

related to #820
have to introduce some new API to IScriptHost and ProjectService because the request is not similar to the existing features. Haven't done the integration test Because I end up using the applyEdit request. I can't find a set of vscode API that can be used to test it. Maybe we can just test it in the server test?

Looking forward to feedbacks if anyone can found some strange behavior.

@jasonlyu123 jasonlyu123 marked this pull request as ready for review February 3, 2021 02:48
@jasonlyu123
Copy link
Contributor Author

I see a new confirm UI in 1.53 insider. I think I can remove the custom confirmation. And we can wait until that was released and use the official confirmation instead.

vscode 1.53 add confirmation for rename file
so change annotation works on will rename
@pakohan
Copy link

pakohan commented Feb 4, 2021

Hey, I'm really looking forward, but never touched development of VSCode plugins. If it's helpful for you, could you tell me how to test this?

Again, thank you for this missing feature :-)

@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Feb 4, 2021

You can check out my branch and follow the doc here.
Following the debug step and you will see a debug vscode window pops up. Just open a folder and you can try it out. You can also use the vscode insider version to debug it.(it's released) And you will see the confirmation I mentioned.

@yoyo930021
Copy link
Member

I'm so sorry about no immediate feedback provided.

I always thought there would be a conflict with VSCode typescript integration.
If you think there is nothing wrong with this, we can try to release it.

Copy link
Member

@yoyo930021 yoyo930021 left a comment

Choose a reason for hiding this comment

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

Thanks for your help.

server/src/modes/script/javascript.ts Outdated Show resolved Hide resolved
server/src/modes/script/javascript.ts Outdated Show resolved Hide resolved
server/src/modes/script/javascript.ts Outdated Show resolved Hide resolved
server/src/services/typescriptService/serviceHost.ts Outdated Show resolved Hide resolved
server/src/services/vls.ts Show resolved Hide resolved
@pakohan
Copy link

pakohan commented Feb 15, 2021

Finally got the time to try it out. It works! I can't wait for the official release :-)

vscode 1.53 would always ask confirmation and provide option for preview
@jasonlyu123
Copy link
Contributor Author

I always thought there would be a conflict with VSCode typescript integration.

About this maybe we can filter out all the edits to js and ts files?

@pakohan
Copy link

pakohan commented Feb 18, 2021

I've checked out the latest commit and might be doing sth. wrong, but it's not working anymore:

Trace: TypeError: Cannot read property 'getProjectVersion' of undefined
    at synchronizeHostData (/Users/mogli/Desktop/vetur-update-import/server/node_modules/typescript/lib/typescript.js:147068:22)
    at getProgram (/Users/mogli/Desktop/vetur-update-import/server/node_modules/typescript/lib/typescript.js:147233:13)
    at Object.getEditsForFileRename (/Users/mogli/Desktop/vetur-update-import/server/node_modules/typescript/lib/typescript.js:147629:45)
    at Object.getRenameFileEdit (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:674:4288)
    at Object.onWillRenameFile (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:3433)
    at /Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:16256
    at async Promise.all (index 0)
    at async GP.onWillRenameFiles (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:16145)
    at Object.getRenameFileEdit (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:674:4968)
    at Object.onWillRenameFile (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:3433)
    at /Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:16256
    at async Promise.all (index 0)
    at async GP.onWillRenameFiles (/Users/mogli/Desktop/vetur-update-import/server/dist/vls.js:858:16145)

@pakohan
Copy link

pakohan commented Feb 18, 2021

btw, is there any way to stop the operation so that the file does not get moved?

@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Feb 19, 2021

I've checked out the latest commit and might be doing sth. wrong, but it's not working anymore:

I can't reproduce this. Is it not working at all? or it's only in a specific situation? Also, I have pulled the latest commits of the master branch to resolve the merging conflict. You'll have to run yarn again.

btw, is there any way to stop the operation so that the file does not get moved?

The confirm windows of vscode 1.53 have a skip option. it would skip all the edits. And you can undo rename file like a normal undo(Ctrl+Z).

If both renamed file and updating file are not vue file,
let typescript extension handle it.
@yoyo930021
Copy link
Member

I always thought there would be a conflict with VSCode typescript integration.

About this maybe we can filter out all the edits to js and ts files?

If the test is not a problem, perhaps we can observe.

Copy link
Member

@yoyo930021 yoyo930021 left a comment

Choose a reason for hiding this comment

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

LGTM, If no any question, I will merge it.

no longer needed to redirect to new file
because this is run before actual file rename
@jasonlyu123
Copy link
Contributor Author

jasonlyu123 commented Feb 26, 2021

Can anyone check it for me? The new test I added works for me on windows. But I just can't figure out why it doesn't work on the Linux test. Open the test/lsp/fixure directory in debugging and move the Basic.vue into a new subfolder. Does it work?
Got reproducible in WSL. Have to do more checking.

we can't use getSourceDoc here. It doesn't have the info we need.
? getUserPreferences(updateCurrentVueTextDocument(sourceFileToSourceDoc(oldPath, sourceFile)).scriptDoc)
: getUserPreferencesByLanguageId(oldPath.endsWith('.js') ? 'javascript' : 'typescript');
const preferences = getUserPreferencesByLanguageId(
(sourceFile as any).scriptKind === tsModule.ScriptKind.JS ? 'javascript' : 'typescript'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't use sourceFileToSourceDoc or getSourceDoc here. It would treat the script content as a vue file. Thus it would get a default script content instead. It only works sometimes because of the caching.
I was hesitant about whether to use this typescript internal API or expose another getScriptKind function from serviceHost

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing you mean getLanguageModelCache......
I also want to remove this cache.

I think we can keep the way you write for now.

Copy link
Member

Choose a reason for hiding this comment

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

@jasonlyu123 If you don't have any problem, I will release it in new version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

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.

4 participants