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

Do not rename imports from ambient modules #8227

Open
mhegazy opened this issue Apr 20, 2016 · 6 comments
Open

Do not rename imports from ambient modules #8227

mhegazy opened this issue Apr 20, 2016 · 6 comments
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

Issue explained by @bbgone in #8118 (comment)

Renaming an import alias for a module coming from a .d.ts file, renames all instances. this is obviously wrong.

Options, 1. error (obviously not helpful), 2. only rename local symbol (better, but leaves the code in an invalid state), 3. rename local import and add as oldname clause to the import declaration as needed (looks like the best solution).

This is similar to the issue related to #7458, except that this issue adds renaming the import alias.

@ghost
Copy link

ghost commented Apr 21, 2016

@mhegazy Option 2 or 3 is fine for me because they are local renaming, and i don't rename at the site of import very often.
As i said that fixes the problem of finding local references of the imports inside the module.
It will also allow easier implementation for showing unused imports as grayed in the code editor because there will be correct API for that.

But if i would use that renaming feature from the site of import i think option 3 is more useful in practice.
P.S. in renaming i always went to the defition of type and renamed it there. I did not even know that it is possible to rename from the site where that type is used.
I rename one time import alias thinking it will be renamed locally, but it renamed it everywhere and there was names collision because that name already was used inside the other modules. That's my experience.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels May 20, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". and removed In Discussion Not yet reached consensus labels Jun 9, 2016
@RyanCavanaugh
Copy link
Member

Approved. The fix here is just to block rename invocations in certain syntactic locations (we can start with this particular one and add more as we find them).

@mhegazy mhegazy added this to the TypeScript 2.1 milestone Jun 9, 2016
@mhegazy mhegazy assigned ghost Jun 9, 2016
@mhegazy
Copy link
Contributor Author

mhegazy commented Jun 13, 2016

This also should cover all symbols declared in .d.ts files, as long as the operation is not initiated from this .d.ts file.

@bryanforbes
Copy link
Contributor

I believe the behavior I'm encountering is relevant to this bug. In a freshly initialized project, renaming things like Object and Number is prevented. Here is the tsconfig.json:

{
    "compilerOptions": {
        "module": "commonjs",
        "target": "es5",
        "noImplicitAny": false,
        "sourceMap": false
    }
}

The file I'm attempting the rename in:

Object.defineProperties({}, {})

The request for the rename of Object:

{"seq":8914211752108017,"type":"request","command":"rename","arguments":{"file":"src/foo.ts","line":1,"offset":1}}

And the response:

{"seq":0,"type":"response","command":"rename","request_seq":8914211752108017,"success":true,"body":{"info":{"canRename":false,"localizedErrorMessage":"You cannot rename elements that are defined in the standard TypeScript library."},"locs":[]}}

This is expected. However, as soon as you add a lib entry to the tsconfig.json, renaming Object is permitted. Here is the tsconfig.json:

{
    "compilerOptions": {
        "module": "commonjs",
        "lib": [
            "es2015"
        ],
        "target": "es5",
        "noImplicitAny": false,
        "sourceMap": false
    }
}

Using the same file as before, here is the request for the rename:

{"seq":4041495444461493,"type":"request","command":"rename","arguments":{"file":"src/foo.ts","line":1,"offset":1}}

And the response:

{"seq":0,"type":"response","command":"rename","request_seq":4041495444461493,"success":true,"body":{"info":{"canRename":true,"kind":"interface","displayName":"Object","fullDisplayName":"Object","kindModifiers":"declare","triggerSpan":{"start":0,"length":6}},"locs":[{"file":"/Users/bryan/Projects/test-rename/node_modules/typescript/lib/lib.es5.d.ts","locs":[{"start":{"line":251,"offset":15},"end":{"line":251,"offset":21}}]},{"file":"src/foo.ts","locs":[{"start":{"line":1,"offset":1},"end":{"line":1,"offset":7}}]}]}}

This also occurs in things included via TypeScript's @types system. At the very least, I would expect that rename requests would be denied ("canRename": false) if changes would occur in files included via "lib", "@types/*", "typeRoots", or "types".

@zpdDG4gta8XKpMCd
Copy link

related #11863

@brandonkal
Copy link

Perhaps this could be expanded to not rename things from node_modules (or at least give a warning) folder?
VSCode decided to rename everything that was of type string and I had to reinstall node_modules. It really shouldn't be reaching into the dependencies folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants