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

Consolidate project files #159

Closed
wants to merge 1 commit into from
Closed

Consolidate project files #159

wants to merge 1 commit into from

Conversation

joeduffy
Copy link
Member

This consolidates the multiple project files we have right now, embellishing
further the Lumi.yaml/json project file as the place for all things project
and package related. Although I suspect we will revist some of this during
next sprint's package management work, this is a step in the right direction.

As a result, we can rip out all the tsconfig.json files scattered throughout.

The new structure adds two top-level sections to the Lumi.yaml/json file:

language:
    compiler: lumixx
    settings:
        a: foo
        b: bar
        z: baz
files:
    - index.xx
    - config.xx

The idea here is that the manifest self-describes three things: 1) the compiler
used to build the package, 2) the settings to pass to that compiler (an opaque
bag to Lumi, since these are specific to the compiler), and 3) the source files
that comprise the project. This helps to unify the different compilers.

This accomplishes work item #49. It also enables #98,
the lumi compile and automatic recompilation commands, which will come soon.

This consolidates the multiple project files we have right now, embellishing
further the Lumi.yaml/json project file as the place for all things project
and package related.  Although I suspect we will revist some of this during
next sprint's package management work, this is a step in the right direction.

As a result, we can rip out all the tsconfig.json files scattered throughout.

The new structure adds two top-level sections to the Lumi.yaml/json file:

    language:
        compiler: lumixx
        settings:
            a: foo
            b: bar
            z: baz
    files:
        - index.xx
        - config.xx

The idea here is that the manifest self-describes three things: 1) the compiler
used to build the package, 2) the settings to pass to that compiler (an opaque
bag to Lumi, since these are specific to the compiler), and 3) the source files
that comprise the project.  This helps to unify the different compilers.

This accomplishes work item #49.  It also enables #98,
the `lumi compile` and automatic recompilation commands, which will come soon.
@joeduffy joeduffy requested a review from lukehoban May 24, 2017 01:42
@lukehoban
Copy link
Member

I definitely like removing some config files.

However, removing tsconfig.json does have the downside of breaking a bit of editor goodness for anyone using an editor that tries to understand TS code. Even vim 😺 . It doesn't totally break these, as Lumi adheres to most defaults. But not having the file list means that things like Find All References can't know the full project context to search, so won't work cross files.

That may be acceptable in order to reduce boilerplate, but it is a notable loss.

@lukehoban
Copy link
Member

Can the tsconfig.json files in the /lib/* folders also be removed after this?

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

LGTM

@joeduffy
Copy link
Member Author

Drats; I do feel like the loss of a good editor experience is a deal breaker. @ericrudder, who is consistently smarter than me, had previously suggested hijacking the tsconfig file, rather than the opposite (which is what I did). Frankly I'm inclined to stick to the "individual files for individual tools" philosophy we have today and which seems to be serving us well (even if it leads to a bit more boilerplate moving pieces).

Thanks for the thoughtful review!

@joeduffy
Copy link
Member Author

I tried out VS Code and verified that the absence of tsconfig.json does indeed break a few things. Most notably, as you say, cross-project operations like Find All References and refactoring. Interestingly, you get good IntelliSense as far as I can tell, because this happens on the consuming side and so presumably the module import leads to parsing and creating a model.

Bummer. Just having a single Lumi.yaml project file would be so much cleaner...

@lukehoban
Copy link
Member

Yeah - the fact that it only break Find All References and Refactoring makes it more tempting to just drop this, or at least not have it be standard, and let folks add it if they want those features.

Note that you can also have an empty tsconfig.json, or just { }, and these project scope features will still work - TS uses some sensible defaults to infer a project rooted in the folder where the tsonfig.json is: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html.

So if we left this file out by default in examples, folks could still add their own empty file if they want the additional IDE features, without any duplication of compiler options or file lists.

@joeduffy
Copy link
Member Author

I'm still mulling this over.

"Accidentally" getting Find All References and Refactoring is actually a pretty cool aspect of our system -- one that I probably vastly under-appreciated since I am stuck in the 1970s editor-wise.

I wish there was some way to get the best of both worlds.

@joeduffy
Copy link
Member Author

I really don't want to lose this great tooling experience, so I'm punting on this for now. More details in #49.

@joeduffy joeduffy deleted the lumi-49-projectfile branch March 2, 2018 16:58
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 this pull request may close these issues.

None yet

2 participants