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

Should "configFileName" be used as filename only or with path? #604

Closed
mengxy opened this issue Aug 16, 2017 · 3 comments · Fixed by #605
Closed

Should "configFileName" be used as filename only or with path? #604

mengxy opened this issue Aug 16, 2017 · 3 comments · Fixed by #605

Comments

@mengxy
Copy link
Contributor

mengxy commented Aug 16, 2017

I am using follow configuration:

{
  configFileName: path.join(__dirname, 'tsconfig.json'),
}

It works as expected in POSIX file systems. But in windows, 'ts-loader' cannot find the config file. Below function in config.js is broken in windows: https://github.com/TypeStrong/ts-loader/blob/master/src/config.ts#L76

function findConfigFile(compiler, searchPath, configFileName) {
    while (true) {
        var fileName = path.join(searchPath, configFileName);
        if (compiler.sys.fileExists(fileName)) {
            return fileName;
        }
        var parentPath = path.dirname(searchPath);
        if (parentPath === searchPath) {
            break;
        }
        searchPath = parentPath;
    }
    return undefined;
}

I just want to discuss that should we use configFileName as simple filename or path. If we use it as filename, then it's fine. But if we can use it as path, we should fixed this bug in windows file system.

@johnnyreilly
Copy link
Member

I'm not sure as I didn't implement this part. @jbrantly any thoughts?

I'd suggest we could fix this for Windows simply and it would be fine - would you be up for submitting a PR?

@mengxy
Copy link
Contributor Author

mengxy commented Aug 16, 2017

Sure, I can follow it up and get it fixed.

Seems that you are using the same findConfigFile function as tsc, but there is some modifications. Maybe that is the issue.

@johnnyreilly
Copy link
Member

Go for it! Thanks!

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 a pull request may close this issue.

2 participants