Skip to content

Replace endsWith() check with isDefaultLibFile() in getRenameInfo() #1990

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

Merged
merged 4 commits into from
Feb 17, 2015

Conversation

jramsay
Copy link
Member

@jramsay jramsay commented Feb 9, 2015

There are a couple of issues with using the current endsWith() function used to determine if we should allow a rename for default lib files:

  1. XXXX-lib.d.ts would not allow renames even though it should as the preceding characters are not being verified for directory separators
  2. There is the potential for false matches as there is currently no check to verify indexOf was successful (index >= 0)

There are a couple of issues with using the current endsWith() function to determine if we should allow a rename for default lib files:
1. XXXX-lib.d.ts would not allow renames even though it should as the preceding characters are not being verified for directory separators
2. There is the potential for false matches as there is currently no check to verify indexOf was successful (index >= 0)
@msftclas
Copy link

msftclas commented Feb 9, 2015

Hi @jramsay, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft employee (Jason Ramsay). If you're full-time, we DON'T require a contribution license agreement.



If you are a vendor, or work for Microsoft Open Technologies, Inc., DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://microsoftcla-south-central-us.azurewebsites.net/.

TTYL, MSBOT;

@mhegazy
Copy link
Contributor

mhegazy commented Feb 10, 2015

I do not think you have this information on the script side. we do not know if lib.d.ts is really a user file, or has been injected into the context by VS. the only one who has this information is VS, and in the genral case, the language service Host.
how the question, should we even do that? or just let the host do that for us? i.e. just return the rename info, and the host will compian if it cannot apply them. alternatively, we can ask the host, if this is trully a library file or not.
if you want to still do this check, i would do something like

ts.getBaseFileName(ts.normalizePath(fileName)) === getDefaultLibFileName(options);

@jramsay
Copy link
Member Author

jramsay commented Feb 13, 2015

Thanks for the comments.
@mhegazy - made the changes we discussed . I was going to add the typeof check for getDefaultLibFileName to ensure it is backward compatible with the previous lower-cased version (getDefaultLibFilename). Case does not appear to matter for the shimHost though as both work (they return typeof "unknown"). Using a completely different method that isn't on shimHost would return typeof "undefined".
I have another change that I'll send a CR out for on the host side if @CyrusNajmabadi and @JsonFreeman are good with this change.

@JsonFreeman
Copy link
Contributor

👍

if (defaultLibFileName) {
for (var i = 0; i < declarations.length; i++) {
var sourceFile = declarations[i].getSourceFile();
if (sourceFile && ts.normalizePath(sourceFile.fileName) === ts.normalizePath(defaultLibFileName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you will need to use getCanonicalFileName here to ensure that case insensitive systems will not have a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point - done

jramsay added a commit that referenced this pull request Feb 17, 2015
Replace endsWith() check with canonical normalized path comparison in getRenameInfo()
@jramsay jramsay merged commit 9788acf into master Feb 17, 2015
@jramsay jramsay deleted the isDefaultLibFile branch February 17, 2015 23:17
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants