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

filterProblemsForDocument() lower-case file paths => breaks diagnostics (OSX) #44

Open
danielweck opened this issue Jul 6, 2017 · 4 comments

Comments

@danielweck
Copy link

danielweck commented Jul 6, 2017

This is the current code:
https://github.com/angelozerr/tslint-language-service/blob/72a02a2/src/index.ts#L99-L110

        function filterProblemsForDocument(documentPath: string, failures: tslint.RuleFailure[]): tslint.RuleFailure[] {
            let normalizedPath = path.normalize(documentPath);
            // we only show diagnostics targetting this open document, some tslint rule return diagnostics for other documents/files
            let normalizedFiles = new Map<string, string>();
            return failures.filter(each => {
                let fileName = each.getFileName();
                if (!normalizedFiles.has(fileName)) {
                    normalizedFiles.set(fileName, path.normalize(fileName));
                }
                return normalizedFiles.get(fileName) === normalizedPath;
            });
        }

The problem is that let fileName = each.getFileName(); returns lower-case file paths! For example, typically on OSX: /User/myName/etc. becomes /user/myname/etc.. No idea why.

I temporarily fixed this by adding .toLowerCase():
let normalizedPath = path.normalize(documentPath).toLowerCase();
...and just for consistency (not strictly needed):
let fileName = each.getFileName().toLowerCase();

@angelozerr
Copy link
Owner

@danielweck thanks for your proposition.

@egamma what do you think about this idea?

@danielweck
Copy link
Author

PS:
I've added this package.json one-liner (sed) NPM script to fix 'tslint-language-service' after each yarn upgrade (or any Yarn / NPM command that resets / updates node_modules):

sed -i "" "s/let normalizedPath = path\.normalize(documentPath);/let normalizedPath = path.normalize(documentPath).toLowerCase();/g" ./node_modules/tslint-language-service/out/src/index.js

(works on OSX, untested elsewhere)

danielweck added a commit to readium/r2-streamer-js that referenced this issue Jul 6, 2017
@egamma
Copy link
Contributor

egamma commented Jul 7, 2017

@angelozerr fixing a file system case sensitivity issue by calling toLowerCase is almost always wrong thing to do, e.g. in a case sensitive file system, there can be a file with a name that is lower case. The typescript server does a lot of effort to handle file systems with different case sensitivity. We should understand why there is a case sensitivity issue on OS X. We should reproduce this is on OS X and see what is the root cause.

@danielweck what are the steps to reproduce this problem?

@danielweck
Copy link
Author

danielweck commented Jul 7, 2017

Sure, create the files below inside a folder called TSLint-test (make sure to use Upper case characters), and run the following commands:

  • cd TSLint-test
  • npm install
  • npm run lint (tslint errors are reported in the console, as expected)
  • npm run lint:full (same errors, as expected)
  • load the folder with VSCode (i.e. code .), open index.ts, wait a second or two ... no TSLint errors are reported in the "PROBLEMS" tab!
  • npm run lintfix
  • close the VSCode app, re-load the folder (i.e. code .), and re-open index.ts => now the TSLint errors show up (red squiggly lines).

index.ts

const txt = 'test'; // TSLINT ERROR (should be double-quotes)
console.log(txt); // TSLINT ERROR (use of console API forbidden)

tslint.json

{
  "extends": "tslint:recommended"
}

tsconfig.json

{
  "languageServiceEnabled": true,
  "compilerOptions": {
    "target": "es2015",
    "module": "commonjs",
    "moduleResolution": "node",
    "outDir": "./dist/",
    "plugins": [
      {
        "name": "tslint-language-service",
        "alwaysShowRuleFailuresAsWarnings": false,
        "ignoreDefinitionFiles": true,
        "configFile": "./tslint.json",
        "disableNoUnusedVariableRule": true
      }
    ],
    "rootDir": "./",
    "rootDirs": [
      "./"
    ],
    "baseUrl": "."
  },
  "exclude": [
    "node_modules"
  ],
  "include": [
    "./*.ts"
  ]
}

package.json

{
  "name": "tslint-lang-service-filepath-lowercase-bug",
  "version": "0.0.0-alpha0",
  "dependencies": {
  },
  "devDependencies": {
    "tslint": "latest",
    "tslint-language-service": "latest",
    "typescript": "latest"
  },
  "main": "./index.js",
  "types": "./dist/index.d.js",
  "bin": {
    "index": "./dist/index.js"
  },
  "files": [
    "dist/**/*"
  ],
  "scripts": {
    "clean": "rm -rf ./dist",
    "lintfix": "sed -i \"\" \"s/let normalizedPath = path\\.normalize(documentPath);/let normalizedPath = path.normalize(documentPath).toLowerCase();/g\" ./node_modules/tslint-language-service/out/src/index.js",
    "prelint": "npm run clean",
    "lint": "tslint -c \"./tslint.json\" \"./*.ts\"",
    "prelint:full": "npm run clean",
    "lint:full": "tslint --project \"./tsconfig.json\" -c \"./tslint.json\" \"./*.ts\"",
    "pretsc": "npm run clean && npm run lint:full",
    "tsc": "tsc -p \"./tsconfig.json\""
  }
}

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

No branches or pull requests

3 participants