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

Is it possible to load tsconfig.json from another directory #539

Closed
santiDotIO opened this issue May 11, 2017 · 10 comments
Closed

Is it possible to load tsconfig.json from another directory #539

santiDotIO opened this issue May 11, 2017 · 10 comments

Comments

@santiDotIO
Copy link

I'm trying to keep all compiler files in one place ./gulp/ but I can't seem to be able to tell ts-loader where to look for tsconfig.json.

@lexey111
Copy link

lexey111 commented Jul 6, 2017

Hi
I have had the same requirement, and I've found the solution. It looks like some... errr... mistake? in path resolution inside ts-loader's config.js file:

 function findConfigFile(compiler, searchPath, configFileName) {
   while (true) {
       var fileName = path.join(searchPath, configFileName);

So one's not always able to specify absolute path to tsconfig.json directly, because it calculates starting path.dirname(loader.resourcePath), and resources (for the case) are placed somewhere outside. BTW, in the ts-lint such case processed with path.isAbsolute(...) and it works fine.

So i'd propose to add the check to the findConfigFile:

function findConfigFile(compiler, searchPath, configFileName) {
   if (path.isAbsolute(configFilename) && compiler.sys.fileExists(configFileName))
      return configFileName;

   while (true) {
       var fileName = path.join(searchPath, configFileName);
       ....

As a workaround, I use context setting for my webpack configuration, and set up the configFileName relative to this context. You can try it for gulp, it has to work.

@johnnyreilly
Copy link
Member

I didn't implement that part and so can't comment. @jbrantly may have a thought. We'd be open to a PR as long as it contains no breaking changes.

@loilo
Copy link
Contributor

loilo commented Aug 25, 2017

TL;DR: This works with the configFileName option (but probably shouldn't).

The option can be passed

  • just a file name, which will be looked for in the entry file's directory and subsequently all parent directories until the file has been found or the file system root was reached.
  • a relative path, which will be resolved relatively to the entry file's directory and subsequently all parent directories until the file has been found or the file system root was reached.
  • an absolute path.

Since #605, passing absolute paths also works on Windows.


However, looking at the original implementation of the findConfigFile function, it appears to me that paths were not initially intended to work with this option.

I assume that because

  1. the option is literally named configFileName (not configFilePath).
  2. resolving of paths works exactly the same way as resolving of file names, which is strange in case of a relative paths and only coincidentally works with absolute paths — on UNIX and with poor performance.

This signals to me that paths should not have worked for this option in the first place but were not properly guarded against.


First, it should be decided by the ts-loader team/maintainer if actual paths (beyond just file names) should be supported by this option at all. Subsequently, this information should find its way into the documentation and the config finder implementation should either be

  • made capable of efficiently resolving all kinds of paths or
  • guarded against paths (i.e. throw an error if a path is provided instead of a file name)

To make the name unambiguous, I'd generally suggest the second option. However, this would break current behaviour (and, obviously, usage) and thus is not desirable at all.


My suggestion:

  1. Fix the current, inefficient lookup chain.

    This should include a minor breaking change, which however has been a bug before anyway: As described above, relative paths are currently resolved relative to the entry file and its parents. This is not a pattern we see anywhere, was probably a bug in the first place and should therefore be avoided.

    It is possible to not introduce this change and only improve the resolving of absolute paths, but I'd strongly advocate against it.

  2. Introduce a configFile option as an alias for configFileName. Document its behaviour.

  3. Deprecate the configFileName option.

  4. Remove the configFileName option with the next major release.


I'd like to hear your thoughts on this. If you like my train of thoughts, I'd be happy to submit a PR.

@johnnyreilly
Copy link
Member

I like your train of thoughts and we'd welcome a PR. Thanks!

@loilo
Copy link
Contributor

loilo commented Aug 25, 2017

Alright, I'll give it a try. 👍

@loilo
Copy link
Contributor

loilo commented Aug 25, 2017

Hm. I get plenty of failing tests even in the freshly cloned original repo. How should I preferredly deal with that?

@johnnyreilly
Copy link
Member

Without seeing it I can't comment. Why don't you send the PR now and keep working on it? Sending the PR will kick off tests on Windows and Linux and hopefully I can give you some feedback.

@loilo
Copy link
Contributor

loilo commented Aug 25, 2017

Alright. Almost forgot that good repos people have a CI setup. 🙃

@loilo
Copy link
Contributor

loilo commented Aug 27, 2017

So this was adressed by #607 and can be closed. 👍

@johnnyreilly
Copy link
Member

Awesome!

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

4 participants