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

[heft] Proposal: Specify tsconfig file to use for heft build/start/test #2258

Closed
paynecodes opened this issue Oct 5, 2020 · 19 comments · Fixed by #2844
Closed

[heft] Proposal: Specify tsconfig file to use for heft build/start/test #2258

paynecodes opened this issue Oct 5, 2020 · 19 comments · Fixed by #2844
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon.

Comments

@paynecodes
Copy link
Contributor

Motivation

Passing the desired tsconfig.json to heft allows several workflows to exist that are either impossible or awkward without the option. Here some reasons this should be taken up:

  1. Separate development (vscode, eslint, other dev tooling that assumes tsconfig.json) and build tools.
  2. Separate development and test tsconfig settings. (Currently, the heft docs/examples recommend including types such as heft/jest in tsconfig which is a red flag, IMO)
  3. Projects that require multiple build variations produced with separate tsconfig options
  4. tsc supports it for these reasons and reasons I can't think of now 🤷

Here an example:

Let's say we have a project with the following folder structure.

src
test
dist
tsconfig.json
tsconfig.build.json

tsconfig.json

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "include": ["src", "test"],
  "compilerOptions": {
    "types": ["heft-jest"]
  }
}

tsconfig.build.json

{
  "$schema": "https://json.schemastore.org/tsconfig",
  // No need to bring in test folder
  "include": ["src"],
  // No need to bring in `heft-jest` types that should not be available for build (for example)
  "compilerOptions": {
    "outDir": "dist",
    "rootDir": "src"
  }
}

Other resources

Workarounds / Other solutions

This could be addressed elsewhere such as going and filing issues with vscode/eslint/other tools that only look for tsconfig.json, and there are good reasons those tools should support an option. Those reasons, for the most part, are the same reasons heft should support an option.

Also, my somewhat contrived example above could be worked around (maybe?) by strategically placing another tsconfig.json file in the test folder. This seems cumbersome and simply will not work for other scenarios.

@halfnibble halfnibble added enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon. labels Oct 5, 2020
@halfnibble
Copy link
Contributor

@BPapp-MS Hello Ben, I think this is the issue we were discussing last Friday. Do you want to look at this and write up a design proposal for this feature?

@octogonz
Copy link
Collaborator

octogonz commented Oct 6, 2020

Separate development (vscode, eslint, other dev tooling that assumes tsconfig.json) and build tools.

This sounds like an implementation choice, not a design requirement. As much as possible, don't we /want/ IntelliSense to accurately reflect the configuration used by the build?

(Currently, the heft docs/examples recommend including types such as heft/jest in tsconfig which is a red flag, IMO)

Jest by design models its APIs as global types. This means that Jest APIs are technically accessible in product files, but would fail at runtime because they don't exist outside the test environment. This is a bit odd, but I've never heard of anyone actually being confused by it. It's not mysterious that describe() and test() will fail if called a product code file.

Thus the importance of // No need to bring in 'heft-jest' types seems debatable.

Projects that require multiple build variations produced with separate tsconfig options

This is already supported today via additionalModuleKindsToEmit, which does not require additional tsconfig.json files. If additionalModuleKindsToEmit isn't sufficient for certain setups, we should dig into that problem, and think more about the design of additionalModuleKindsToEmit.

// No need to bring in test folder

Reducing the number of files compiled by heft build is an interesting topic. Today, Heft compiles all your source files (*.ts and *.test.ts) one time, in a single pass. Jest and Webpack then are invoked on the output *.js files. In a project with 100% code coverage, the amount of test code could easily exceed the amount of non-test code. So not compiling test files would very likely be a big speed gain for rush build --lite. That seems worth tackling.

File selection is a simple enough problem that, by itself, does not require a separate tsconfig.json file.

If there is a separate tsconfig.json file, perhaps it doesn't need to do this:

"include": ["src", "test"],

For example, you mentioned...

my somewhat contrived example above could be worked around (maybe?) by strategically placing another tsconfig.json file in the test folder.

C# actually models unit tests as its own DLL project that gets compiled separately, with a dependency on the main application assembly. I always thought it was slightly odd that TypeScript projects mix together *.ts and *.test.ts and then have to pick them apart using .npmignore (being careful to omit test assets).

If we separated them, then in theory heft build could compile *.ts, and then a multi-phase heft test could come along and compile *.test.ts without having to recompile the *.ts files. Down the road, such a design might have other advantages... 🤔

@BPapp-MS
Copy link
Contributor

BPapp-MS commented Oct 6, 2020

This sounds like an implementation choice, not a design requirement. As much as possible, don't we /want/ IntelliSense to accurately reflect the configuration used by the build?

@octogonz The reason that I would need the feature for my project, and that I imagine others needing the feature, is that it becomes easier to transition into strictNullChecks for large codebases if you're able to show those mistakes in VS Code while not enforcing them in the compiler. It pairs with the fact that VS Code also only supports reporting errors from the tsconfig.json, and this seems to be a common enough problem that I'm seeing others using their vanilla tsconfig purely for intellisense as well, with others for their actual tsc tasks. Another blog post addressing the idea of ramping up strictNullChecks on specific files, also wouldn't work for us it seems, as they're enforcing them for specific files with a separate tsconfig that targets just those files. It seems that that wouldn't add the Intellisense, still, but we wouldn't be able to support that workaround either, as it stands right now. In any case there's definitely a desire in the community to fine-tune Intellisense in VS Code and Heft users wouldn't be able to do that while both Heft and VS Code strictly use the vanilla tsconfig.json.

@paynecodes
Copy link
Contributor Author

As much as possible, don't we /want/ IntelliSense to accurately reflect the configuration used by the build?

Yes, but not at the expense of flexibility where it's needed. As much as possible is fine, but I'm not suggesting that each config would diverge more than is necessary. In fact, the way I actually do this is have a tsconfig.base that tsconfig and tsconfig.build extend from.

Thus the importance of // No need to bring in 'heft-jest' types seems debatable.

This is just a concrete example. Thinking of my examples more generously as classes of problems is likely to lead to us down a more productive path.

This is already supported today via additionalModuleKindsToEmit, which does not require additional tsconfig.json files.

I need to dig deeper into this. Thank you for pointing this out.

C# actually models unit tests as its own DLL project that gets compiled separately, with a dependency on the main application assembly. I always thought it was slightly odd that TypeScript projects mix together *.ts and *.test.ts and then have to pick them apart using .npmignore (being careful to omit test assets).

I could be swayed one way or the other on this. Both options should be possible. Right now, I prefer colocating my unit tests with my source code. I also prefer to colocate my Storybook story files for UI component libraries. A package with 50 UI component folders, another package with the same 50 UI component test folders, and another package with the same 50 UI component story folders sounds pretty unruly to me. I take your point about the ignore globs, but, to each his own.

Opinions about testing, IntelliSense, tooling such as Storybook, doc builders, etc. will be all over the place. I think an option here is valid for every reason stated in OP.

I'm struggling to steel man the arguments for not allowing, encouraging even, passing separate compilation options similarly to how the first party tool, tsc, allows. Perhaps someone can take a stab at it.

@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2020

I'm struggling to steel man the arguments for not allowing, encouraging even, passing separate compilation options similarly to how the first party tool, tsc, allows. Perhaps someone can take a stab at it.

The first party tsc is not part of an optimized toolchain. It doesn't support preprocessors like quicktype. Its "watch mode" doesn't consider Jest or Webpack. If you look at Heft's TypeScriptBuilder.ts, it does a bunch of caching and special optimizations that are not implemented by tsc.

So (waving hands around vaguely) the main concern with multiple tsconfig.json profiles is potential interactions with all of this complex logic. That isn't a reason NOT to do it. But it is a reason to think carefully about why are we doing it, and what's the best approach.

This is just a concrete example. Thinking of my examples more generously as classes of problems is likely to lead to us down a more productive path.

If we can find one really compelling example for supporting tsconfig.build.json, that would go a long way towards pushing this forward. It can be difficult to design for a broad "class of problems", whereas solving one really clearly articulated problem is often pretty easy.

@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2020

The reason that I would need the feature for my project, and that I imagine others needing the feature, is that it becomes easier to transition into strictNullChecks for large codebases if you're able to show those mistakes in VS Code while not enforcing them in the compiler.

@ BPapp-MS Speaking as someone who spent several excruciating nights fixing up thousands of files to conform to stricter tsconfig.json checks, this sounds like a pretty good approach. :-)

Lots of tools look at tsconfig.json -- ESLint, API Extractor, TSDoc, etc. For an IntelliSense-only configuration, wouldn't it make more sense to put that in .vscode/settings.json as a recommended editor configuration? Or for VS Code to natively recognize a special filename like tsconfig.editor.json? Has someone proposed this to VS Code? What did they say about it?

@paynecodes
Copy link
Contributor Author

paynecodes commented Oct 12, 2020

@octogonz Yes, vscode should allow customization.

Here's the issue where it was discussed.

@alfonsoar
Copy link
Contributor

Hi All - I think this would be a really nice feature to have, I wrote up my specific use case here #2297 (comment)

@BPapp-MS
Copy link
Contributor

@octogonz I've made the following feature request to VSCode: Allow for highlighting of TypeScript strictness features NOT via tsconfig. This would address the problem of being able to see things like strictNullChecks in the editor without having to alter the tsconfig in ways that would break the heft compilation. Getting them to read from alternative tsconfig files seems unlikely, as that thread ended with the following comment:

I've locked this issue because many of recent comments are not constructive and are just polluting the conversation. Suffice it to say: if doing this were as easy and had zero repercussions, we would have already done it

@paynecodes
Copy link
Contributor Author

paynecodes commented Apr 1, 2021

OP here to show another example where this will be useful.

Here is a next.js example where they are suggesting a tsconfig.json and a tsconfig.server.json file. I suppose the only answer for now is to drop down to the tsc compiler instead of relying on heft to support this option.

Let me know if I'm missing something.

@dmichon-msft
Copy link
Contributor

Currently the Heft TypeScript plugin blindly and greedily assumes that any tsconfig-*.json files in the root directory are meant for its consumption, and that it should run all of them. This is clearly ill-behaved, and the feature ask here would be a way to specify in either the CLI or typescript.json (or both) exactly which config is meant for Heft. This makes it a better-behaved citizen in the ecosystem, where most other tools (ts-loader, ts-jest, typescript, tslint) all support specifying which tsconfig should be used directly.

@apendua
Copy link

apendua commented Jul 19, 2021

@dmichon-msft

Currently the Heft TypeScript plugin blindly and greedily assumes that any tsconfig-*.json files in the root directory are meant for its consumption ...

This is great! However, I am wondering if this is some sort of documented (and supported) feature. I've just tried that and it doesn't seem like tsconfig-es5.json actually gets processed by Heft at all. I suppose I am missing something in my configuration.

@apendua
Copy link

apendua commented Jul 19, 2021

@dmichon-msft I did some code reading and it looks like you dropped this functionality recently:

5f2f944

@paynecodes

Here is a next.js example where they are suggesting a tsconfig.json and a tsconfig.server.json file.

I believe that the usability of this goes far beyond next.js. Imagine I am package author and I would like my published package to be consumable by both webpack and latest node, say 14.x. Because node already supports most of the recent ES features, I would probably like to use es2017 as the target. However, for better old browsers compatibility it still makes sense to consider es2015.

My idea was to have two parallel tsconfig files, one of which would output to lib and the second one to lib-es5 and I would define two entry points in package.json, e.g. main: "lib/index.js" and browser: "lib-es5/index.js". This seems like a reasonable approach and a pretty common use case. Unfortunately, this is currently not doable with heft.

I suppose the only answer for now is to drop down to the tsc compiler instead of relying on heft to support this option.

Sure, this is a possibility but I am wondering if there could be a better solution for something which really seems like a very typical scenario.

@idahogurl
Copy link

I have the same issue with Next.js. I can't get heft test to work because Next.js wants tsconfig.json to use "jsx": "preserve" and not "jsx": "react"

@Faithfinder
Copy link
Contributor

I'll add my voice I guess.

Our usecase is similar to some of the already described - we're migrating a large-ish JS database to typescript. We have noImplicitAny enabled in tsconfig for editor support, but we also have a tsconfig.build where it's disabled, because it's practically impossible for us to type out everything in a single development cycle.

@paynecodes
Copy link
Contributor Author

@apendua

I believe that the usability of this goes far beyond next.js. Imagine I am package author and I would like my published package to be consumable by both webpack and latest node, say 14.x.

I certainly agree that this would be useful for much more than next.js. In my opinion, the arguments for this feature are far stronger than against it.

@dmichon-msft
Copy link
Contributor

Added "project" option in typescript.json with identical semantics to the tsc --project parameter in #2844.

@apendua
Copy link

apendua commented Aug 10, 2021

@dmichon-msft Thank you so much for working on this!

I tried to use the approach to support my use case, i.e. multiple build targets in separate folders, e.g. lib and lib-es5.
If I understand correctly, I should include the secondary tsconfig file in references and set buildProjectReferences: true in my typescript.json. The only concern I have is that the use of references introduce some additional behavior, which may not always be desirable. It also seems like this particular tsc feature is designed to glue separate projects rather than compiling a single one with various settings.

I am now wondering if it would be more useful to have a list of projects in typescript.json rather than a single reference to a custom tsconfig file name.

@dmichon-msft
Copy link
Contributor

The way you would use this for that scenario is to have the typescript.json refer to a "solution" tsconfig that contains an empty files array and empty include array (for completeness), and project references to the configs for building with each target and outdir. That said, that wasn't really the design intent of the feature.
Supporting multiple emit targets would be more akin to the existing additionalModuleKindsToEmit feature.

@iclanton iclanton moved this to Closed in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change needs design The next step is for someone to propose the details of an approach for solving the problem priority The maintainers consider it to be an important issue. We should try to fix it soon.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

9 participants