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

allow baseUrl, paths, rootDirs in tsconfig files #849

Closed

Conversation

laurelnaiad
Copy link

Re: #833, Add the above as valid tsconfig.json properties.

Note: this change does not necessarily make these properties functional, it
simply allows them to be passed to the transpiler.

baseUrl and paths seem to work. rootDirs has not been verified and
might not be functioning (at least on cur. typescript@next).

TODO: this does not yet deeply validate the paths and rootDirs properties.

I plan to mark this as closing the issue if I can figure out what's up with rootDirs as time permits, but this might be merge-able now as a starting point.

Add the above as valid tsconfig.json properties.

Note: this change does not necessarily make these properties functional, it
simply allows them to be passed to the transpiler.

`baseUrl` and `paths` seem to work. `rootDirs` has not been verified and
might not be functioning (at least on cur. typescript@next).

TODO: this does not yet deeply validate the paths and rootDirs properties.
laurelnaiad referenced this pull request in laurelnaiad/atom-typescript Feb 14, 2016
Add the above as valid tsconfig.json properties.

Note: this does not yet deeply validate the paths and rootDirs properties.

Closes TypeStrong#833
@mko
Copy link

mko commented Feb 22, 2016

+1

@basarat
Copy link
Member

basarat commented Feb 23, 2016

@laurelnaiad I've added you as an owner. Please do whatever you want with this package. Merge then publish https://github.com/TypeStrong/atom-typescript/blob/master/CONTRIBUTING.md#publishing then write notes at https://github.com/TypeStrong/atom-typescript/releases 🌹

Feel free to @ mention me if you need any help.

@laurelnaiad
Copy link
Author

@basarat !? :(

Yikes. I'll do what I can.... does this mean that you're really backing off on atom-typescript? I can't carry your load.... more hands, sharper tacks needed..... I will have questions for you when I have some breathing room from project work.

@basarat
Copy link
Member

basarat commented Feb 23, 2016

I can't carry your load

Don't worry you are just one of the many people. Just means that I trust your code explicitly and don't feel the need to review it 🌹

does this mean that you're really backing off on atom-typescript?

I'm trying to limit my OSS work, sticking to StackOverflow + alm.tools + typescript book. Focus for this year is revising Algorithms and Data Structures 🌹

@laurelnaiad
Copy link
Author

Ok... I'll look at the rootDirs situation tomorrow and see if I can finish this off with the accoutrements that an owner would provide.... :)

@laurelnaiad
Copy link
Author

I'm developing a theory that this PR leaves behind an issue relating to LanguageServiceHost(2) losing track of files. I'm wondering whether it's receiving partial paths where it clearly wants absolute ones.

All I know for sure so far is that my instances of "The file '/Users/dmaddox/.../somewhere/suchAndSuch.ts' is not included in the Typescript compilation context. If this is not intended..." have gone way up. Hopefully I'll have some time soon to dig into this.

@mko
Copy link

mko commented Feb 26, 2016

I locally had implemented this same concept slightly differently than in your PR, and I saw the same problem. If I have time on Monday, I'll try to dig into resolving those issues with you.

Michael Owens
mk@mowens.com
https://mowens.com
@mko

On Feb 26, 2016, at 10:55 AM, Daphne Maddox notifications@github.com wrote:

I'm developing a theory that this PR leaves behind an issue relating to LanguageServiceHost(2) losing track of files. I'm wondering whether it's receiving partial paths where it clearly wants absolute ones.

All I know for sure so far is that my instances of "The file '/Users/dmaddox/.../somewhere/suchAndSuch.ts' is not included in the Typescript compilation context. If this is not intended..." have gone way up. Hopefully I'll have some time soon to dig into this.


Reply to this email directly or view it on GitHub.

@laurelnaiad
Copy link
Author

Strong circumstantial evidence that non-rooted paths are sneaking their way into places they shouldn't be:

This is an exerpt from having done the following logging in the source:

app/routes/layout/layout.ts
app/routes/root/root.ts
/Users/dmaddox/Documents/_ws/myproj/src/browser/app/ng1Providers.ts
/Users/dmaddox/Documents/_ws/myproj/src/browser/node_modules/rxjs/add/observable/zip.d.ts
/Users/dmaddox/Documents/_ws/myproj/src/browser/app/app.ts
/Users/dmaddox/Documents/_ws/myproj/src/browser/app/appState.d.ts

from project.ts

    /** all files except lib.d.ts  */
    public getProjectSourceFiles(): ts.SourceFile[] {
        var libFile = languageServiceHost.getDefaultLibFilePath(this.projectFile.project.compilerOptions);
        var files
            = this.languageService.getProgram().getSourceFiles().filter(
              x => {

                console.error(x.fileName) // laurelnaiad's addition
                return x.fileName !== libFile
              }
        );
        return files;
    }

Those non-rooted paths just happen to be some of the ones that are being reported as "not in a compilation context".

@laurelnaiad
Copy link
Author

I find it slightly odd, I guess, that in the presence of tsconfig files that have files arrays, that we'd be asking tsc for a list of files in the project... but I guess there are reasons. In any case, I guess the fix is to resolve to rooted-paths... wherever that lands is probably also a reasonable place/time to address rootDirs, I suppose.

@basarat basarat mentioned this pull request Mar 2, 2016
@tomitrescak
Copy link

👍 Can't wait for this one! ;)

@laurelnaiad
Copy link
Author

Me, too! :) I tried hacking at the getProjectSourceFiles function to see if teaching it how to resolve the paths would help, but apparently that function is also used in place where finding the files leads to confusion in tsc itself (assertion errors that we're looking for a file that doesn't exist)... so apparently I messed with the wrong function...

I am going to try to carve out a little time this weekend to find an alternative function to hack. :)

@basarat basarat mentioned this pull request Apr 8, 2016
@@ -234,6 +240,9 @@ export var defaults: ts.CompilerOptions = {
target: ts.ScriptTarget.ES5,
module: ts.ModuleKind.CommonJS,
moduleResolution: ts.ModuleResolutionKind.NodeJs,
baseUrl: undefined,
paths: undefined,
rootDirs: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

No need to provide defaults if they are not activated (basically let the TS compiler do its own default thing). Also explicit undefined initialization not needed (it undefined on read anyways) 🌹

@basarat
Copy link
Member

basarat commented May 14, 2016

I just added the support to http://alm.tools : alm-tools/alm#73 🌹

The only thing missing from this PR I think is that baseUrl isn't being make absolute as soon as its read from the the tsconfig.json file (see other examples like outFile in the same code here https://github.com/TypeStrong/atom-typescript/blob/de5aca96840dfc8723652f115d17341bf217320d/lib/main/tsconfig/tsconfig.ts).

@basarat
Copy link
Member

basarat commented May 14, 2016

rootDirs would just work too if the path resolution is done up front. See alm-tools/alm#74 for commit 🌹

@talski
Copy link

talski commented Jun 14, 2016

Hello guys, any news when this will come out?

@PatrickJS
Copy link

+1

@basarat
Copy link
Member

basarat commented Jul 14, 2016

Superceeded by (now merged) #996 🌹

@basarat basarat closed this Jul 14, 2016
@TypeStrong TypeStrong locked and limited conversation to collaborators Jan 20, 2019
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.

None yet

6 participants