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

Suggestion: Rename imports in modules locally adding an import allias #8213

Closed
ghost opened this issue Apr 20, 2016 · 10 comments
Closed

Suggestion: Rename imports in modules locally adding an import allias #8213

ghost opened this issue Apr 20, 2016 · 10 comments
Labels
In Discussion Not yet reached consensus

Comments

@ghost
Copy link

ghost commented Apr 20, 2016

At present if we rename an import in Visual Studio Code editor it renames globally throughout the project and that includes the site of the definition.

For example if we rename BaseObject to MyObject in the following import declaration:

import { BaseObject } from "jriapp_core/object";

The uses of BaseObject will be renamed not only in the current module but in all other modules too.
If BaseObject was defined in type definition file object.d.ts it will be renamed too.
That means that after type def regeneration the project is broken.

So i suggest if we rename an import (as BaseObject to MyObject) then only add an alias.

import { BaseObject as MyObject } from "jriapp_core/object";

This is correct because imports are actually function arguments, so they are local to the module which imports them. In the case of importing type declaration as interfaces, they are just provided by reference and we rename only the reference's name - not the actual type's name.

That behavior (at present) also prevents us to find all references for the import locally, because it finds them in the whole project, and that is not useful to anybody at all.

P.S.: and this is my comment from another issue #8118 which i appended after it had been closed.

_if we look more closely at renaming at the site of import instead of renaming at the site of declaration then we can see how things can be broken easily.
For example: we could have a definition file app.d.ts, and imported some types or variables into module1.ts, then at the site of import in module1.ts we rename that imported type and this will rename it to in the definition file too. But after regenerating the definition file our application is broken!
Another example, if we rename at the site of the import and that import is used in other modules too and the other modules use local variables with exactly the same name as we will rename in the first module, then we get a name collision in other modules - the imports will have the names of local variables or exports.
If we renaming at the site of declaration then we are aware of these possibilities, but if we rename at the site of import we are getting into it unawarely.
_

@billti
Copy link
Member

billti commented Apr 20, 2016

Discussing this in the room, there's some interesting implications and points of disagreement. I personally often rename an imported identifier and expect it to cascade to the module where I exported (and to other imports), so I'm happy with the current behavior. But some folks are in agreement with you that this is a local identifier and should be aliased by a rename, and you should have to rename at the definition to cascade throughout the project. Perhaps we need two operations here (e.g. rename local, and rename global).

There are some other interesting related points to this. For example, renaming that impacts a .d.ts file. Again, I often hand-author one to describes types I want to use throughout a project, so I expect my rename at a use point to rename in the .d.ts also. However, as you point out, a .d.ts file may be generated, and thus doing a rename in it simply get overwritten when it's next generated. There is no way to tell currently if a .d.ts is generated or hand-written (perhaps we could add a preamble/comment to indicate if generated, and refuse to rename in a generated .d.ts?)

You could make a similar argument for other scenarios too - e.g. if a class implements an interface and you rename an implemented member, then do you intend to rename the member on the interface (and all other places it is implemented)? Or just rename the method on the class (and end up with an error as you now don't implement the interface any more). Again, seems like there are really two operations here (rename global or local), and having both may be more obvious/intuitive than requiring folks go to the declaration point and rename if the intent is for a global rename.

Adding 'in discussion' until we arrive at a conclusion.

@billti billti added the In Discussion Not yet reached consensus label Apr 20, 2016
@ghost
Copy link
Author

ghost commented Apr 20, 2016

@billti The problem that the global renaming prevents to find references locally to imported parts.
Because renaming relies on "Find All references" behaviour.
The question is it useful in a current module to click on imported part to find all references and obtain the references for the imported type in the whole project instead of only in the current module. It is useless. To find local references is very frequently used operation, but i can not do it with current behavior and i'm stuck with the "find and replace" dialog to look for the text.

P.S. and implementing my suggestion the bug #8171
is fixed automatically!!!

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

I would say most of the time module imports should be not be renamed. this makes it easier to follow the logic, specially with reexports (i.e. export * from and export {..} from). In that spirit, a rename should be a global operation rather than a local one.

The API has an entry point for local references, which is occurrences, it is visible in IDEs today in the form of highlighting the text.
image

I would expect your request for a new gesture to make a local rename, be exposed in the IDE. supporting this new API on the TS side should be trivial. i just do not think that we should change the behavior for all users of the existing rename API.

@ghost
Copy link
Author

ghost commented Apr 20, 2016

My most problem that i can not do local "Find all references".
It produces a bunch of references in the whole project.
I suggested it to fix this issue.
P.S. I think all of the users would be greatful if you implemented this.
Because it is correct behaviour and very much expected.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

My most problem that i can not do local "Find all references".

that is what i am saying too, you need a new API entry point, a new gesture to do "local rename"/ "local find references". And to do that, you need to ask VSCode to support that. adding API on TS for this should be fairly trivial.

@ghost
Copy link
Author

ghost commented Apr 20, 2016

Where is it VSCode? I have no contacts with them.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

sorry, i assumed you are using VSCode as the main IDE. so what is the IDE you are currently using?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

@BBGONE would #8227 cover your scenario?

@ghost
Copy link
Author

ghost commented Apr 21, 2016

@mhegazy Yes, it is!

@mhegazy
Copy link
Contributor

mhegazy commented Apr 22, 2016

thanks! closing in favor of #8227

@mhegazy mhegazy closed this as completed Apr 22, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
In Discussion Not yet reached consensus
Projects
None yet
Development

No branches or pull requests

2 participants