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

RFC: Target-project based control of TypescriptConfig from TypeScriptProject #3448

Open
2 tasks done
giseburt opened this issue Mar 13, 2024 · 14 comments · May be fixed by #3535
Open
2 tasks done

RFC: Target-project based control of TypescriptConfig from TypeScriptProject #3448

giseburt opened this issue Mar 13, 2024 · 14 comments · May be fixed by #3535

Comments

@giseburt
Copy link
Contributor

giseburt commented Mar 13, 2024

Problem: One of the many elements being managed by TypeScriptProject is the TypescriptConfig, providing an opinionated default compilerOptions that are then expected to be overridden. TypeScript is used for a wider and wider range of projects, and the opinionated compilerOptions only covers a static subset of those.

We don't want to otherwise change how TypeScriptProject is managing the TypescriptConfig, only compilerOptions and extends under certain circumstances.

Those defaults cover a somewhat narrow target type (node app, basically), which I’m not complaining about, but is somewhat brittle and puts the maintenance burden on this project. Each project subtype (such as a React one) provides overrides, which is great, and this should make that easier as well.

I'm proposing to use the @tsconfig project maintained settings, choosing a base and optionally extending other sets like strictest while still allowing overrides.

So, conversationally that would read as: “I’m building for Node20, would like the strictest settings, except exactOptionalPropertyTypes, which I find annoying and unhelpful.”

Or, “I’m building for React in a browser, and then bundling it. I'll be using Node18 locally, and need my test code to be strict.”

That last one brings up the element of tsconfig.dev.json, which is for running projen, local utilities that use typescript (eslint, jest/ts-jest, etc.), and may have wildly different settings than the target in tsconfig.json. For this reason, we should also have the ability to say that tsconfig.dev.json doesn't necessarily extend or even resemble tsconfig.json.

Proposal: One possible way of doing this is via a buildTarget option in TypeScriptProjectOptions, with enum values for the various targets, including @tsconfig options: PROJEN_CLASSIC (current defaults), NODE18, NODE20, REACT_NATIVE, etc. For local tools (tsconfig.dev.conf) perhaps runTarget would work, defaulting to PROJEN_CLASSIC?

Should also have the ability to add the strictest set. Maybe something like buildTargetOptions: { strictest: true }?

Further, there should probably be an option for "full control" - UNSPECIFIED perhaps?

With that in mind, the code for those conversations:

  • “I’m building for Node20, would like the strictest settings, except exactOptionalPropertyTypes, which I find annoying and unhelpful.”
    const project = new TypeScriptAppProject({
      //...
    
      buildTarget: TypeScriptTarget.NODE20,
      buildTargetOptions: { strictest: true },
      tsconfig: {
        compilerOptions: {
          exactOptionalPropertyTypes: false, // just annoying and unhelpful
        },
      }
    });
  • “I’m building for React in a browser, and then bundling it. I'll be using Node18 locally, and need my test code to be run with strictest.”
    const project = new TypeScriptAppProject({
      // ...
    
      // building for the browser, remote setup
      buildTarget: TypeScriptTarget.REACT,
      tsconfig: {
        compilerOptions: {
          moduleResolution: TypeScriptModuleResolution.BUNDLER,
        },
      },
    
      // local setup
      runTarget: TypeScriptTarget.NODE18,
      runTagetOptions: { strictest: true },
      tsconfigDev: {
        compilerOptions: {
          exactOptionalPropertyTypes: false, // just annoying and unhelpful
        },
      }
    });

Possible tasks:

@moltar
Copy link

moltar commented Mar 25, 2024

I am not a fan of abstracting away tsconfig to a third-party.

Projen is the config manager. By moving away the responsibility of the config upstream, then each projen-based project is at risk of configuration shifting upstream. It breeds an action-at-a-distance type of problem.

And @tsconfig specifically had introduced breaking changes in the past without warning.

When I have the full config locally, I can easily see at a glance what can be tweaked and/or what is wrongly configured.

And when it's extending some other one, it's not that simple. You must lookup inside node_modules/@tsconfig/base... and even then, they may inherit from another package, so you need to resolve it.

Or use tsc command to resolve full config, but which then also includes every default and is very noisy.

@giseburt
Copy link
Contributor Author

@moltar Thats a fair assessment. I see what you're saying about trusting and relying on someone else.

The '@tsconfig' configurations are really just an example of a curated set of configs for a variety of targets. There may even be others to choose from.

I think providing our own set of curated options works as well, would in fact be ideal. I didn't want to suggest that burden necessarily be picked up by this project. If this project is accepting of that responsibility, then that's all the better.

I'd say being able to choose is a very important part. Even if we choose to curate our own set of configurations, I think it would be in our best interest to allow choosing from outside options as well. I don't see it being a risk as long as it's for the application code.

The only place I'd be uncomfortable with broad choice is for compiling and execution of .projenrc.ts itself. For .projenrc.ts I'd presume we want to limit the options.

@mrgrain
Copy link
Contributor

mrgrain commented Mar 26, 2024

The only place I'd be uncomfortable with broad choice is for compiling and execution of .projenrc.ts itself. For .projenrc.ts I'd presume we want to limit the options.

I'm off the relatively strong opinion that .projenrc.ts should follow the same standards you are following in the rest of the project and thus use the same tsconfig. There's a good argument to align it with the tsconfig that is used for tests (not production code) if these are different, however I'm not sure that's normally the case (jsii projects aside).

@mrgrain mrgrain pinned this issue Mar 27, 2024
@giseburt
Copy link
Contributor Author

giseburt commented Mar 28, 2024

Well, it's worth breaking it down with a few example scenarios I've encountered:

Project Target Can Share?
jsii project or subprojects (CDK, etc) jsii compiler makes the tsconfig for us 👎
nextjs-ts, react-ts JavaScript in the browser, es5 for React and bundler for NextJS 👎
nests-ts JavaScript in the browser AND node via their own bundler, has very specific config options 👎
typescript for a published npm package likely a variety of node versions, and possibly both ESM and CJS (maybe via bundler) 👍
typescript for a VS Code extension the node version built into vscode 👍
typescript-app for an AWS Lambda the node version in the Lambda Runtime, likely bundled 👍

The 👍 indicates that we can use the same TS configuration for the application code as for projen and testing. There are arguments for and against that sharing, with merits case-by-case.

The 👎 indicates that they cannot be shared since the targets are incompatible.

In all 👎 cases and several 👍 cases, the tsconfig.dev.json will not or should not share certain settings, such as target, module, etc. That can share other settings, such as strictness and type-handling rules when possible just to maintain a similar coding style.

Tests are tricky, as you want them to match the target environment, but you have to run them with the tooling of the project, so I think they have to fall under either the tooling config (tsconfig.dev.json) or a third config, but that seems unnecessary. For cases like the web-targeted options, there's special tooling like puppeteer to emulate the web environment. In that case the code running in puppeteer is the source, and the code running puppeteer is local and node.

@giseburt
Copy link
Contributor Author

I'm off the relatively strong opinion that .projenrc.ts should follow the same standards you are following in the rest of the project and thus use the same tsconfig.

Which brings to mind something I haven't thought of (or heard of) before, which would be splitting the tsconfig settings into two (maybe more?) categories, and providing option groups of those categories that you mix and match. Categories such as strictness, style, and target. Not just considering it all just target.

Conversationally, that would read something like: "We're going to make a react-ts project, the tsconfig settings will be the strictest-reasonable (exactOptionalPropertyTypes is not reasonable), and the source will target es6 while we run with node20 tooling locally and in the pipeline"

I realize that @tsconfig has some of this, with strictest, but I'm thinking maybe multiple middle-ground options. I honestly don't know what settings one could call "style," but that does bring to mind we have a similar situation with eslint configs that could take hints off of what we're doing here.

I'm less sure about this part, but if we're intending to curate settings groups, it would be good to at least separate "strictness rules" from "target rules".

@mrgrain
Copy link
Contributor

mrgrain commented Mar 28, 2024

jsii compiler makes the tsconfig for us

The actual reason here is that the tsconfig is used to discover files included in the jsii assembly and projen code should not be included. The config itself is perfectly capable of compiling projen code.

JavaScript in the browser, es5 for React and bundler for NextJS
JavaScript in the browser AND node via their own bundler, has very specific config options

I'm not entirely sure I follow why this would prevent projen code from being run, but I can accept that there are cases where it's not desirable to share the config. 👍🏻

@mrgrain
Copy link
Contributor

mrgrain commented Apr 1, 2024

I though more about this and I'm not convinced we need to introduce a new concept of a "target".
The @tsconfig project seems interesting, but I'm also not convinced projen is benefiting much from directly depending on it, while inheriting all the risk that @moltar has pointed out.

The core of the issue seems to be that currently tsconfig (and tsconfigDev respectively) are badly name since they really represent something like tsconfigExtendWith.

I think we should take a step back and look at how we can model these properties while satisfying the following requirements:

  • (1) A project can provide default values for tsconfig
  • (2) A user of this project can amend these default values, that is providing different and additional values to these defaults
  • (3) 🆕 A user can completely "remove" all defaults without having to know the exact configs that the project provides, that is it will also remove any feature configs that the project might decided to provide.
  • (4) 🆕 A user can provide a completely new tsconfig that doesn't use any of the defaults the project provides
  • (5) 🆕 A component/project developer can offer a reusable way to use some other tsconfig defaults, e.g. using the @tsconfig project. These would be used by a user via (4).
  • (6) 🆕 Crucially (3) and (4) do NOT have to be usable together, i.e. (3) and (4) are an "one or the other" choice the user has to make. Although maintainers of (5) can choose to implement (2) + (3) for their components.

I believe there is one more requirement that you can be distilled out from the conservations, but I'm not entirely sure yet about its value and if I understand the need correctly. It might also just be an extension of (3). So please @giseburt if you could assist to set this out correctly:

  • 🆕 A user can remove a project's default compilerOptions while retaining the other properties of the tsconfig. This is because other properties like files and include are more universally useful than compilerOptions.
    • A generalization of this could be that any properties at any nesting level can be selectively reset.
    • It might even be that for (3) it would be acceptable that top-level props have to be "un-defaulted" manually, given that tsconfig only has a limited number of them. Although that'd be less preferable.

With the exception of (3) I believe these are already achievable with today's API, but super hacky. We are looking for a new API that can satisfy all the above requirements (once confirmed). We then also need to worry about migration, but let's make this a secondary concern.

@giseburt
Copy link
Contributor Author

giseburt commented Apr 2, 2024

Good breakdown, @mrgrain.

If we think in terms of L1, L2, L3 constructs based on those defined for CDK, briefly defined as:

  • L1 - provide a single configuration or element in the project1 - such as a YAML file, section of a package.json, or task definition
    • e.g.: FileBase and anything that extends it directly, and TypescriptConfig since it manages the contents of a JSON file
  • L2 - provide a higher-level abstraction through an intuitive intent-based API
    • e.g.: TypeScriptProject provides sensible defaults and allows you to describe intent
  • L3 - provide patterns by utilizing a collection of resources that are configured to work together to accomplish a specific task or service
    • e.g.: AwsCdkTypeScriptApp provides a pattern of a sensibly configured CDK app, orchestrating typescript, eslint, GitHub workflows, finding and making Lambda functions, etc.

One could argue that TypeScriptProject is an L3, which is somewhat true, as it also provides a pattern, but it's more generic. I think of it as much like the CDK L2 ec2.Instance, which provides the functional minimum for an EC2 instance to work based on the stated intent (with sensible defaults) which requires also making roles, security groups, etc., and wiring them all together.

With that in mind, I believe the part I think this RFC is most concerned with is the creation of L3 constructs, including "apps" (.projenrc.{ts,py,etc}) that are the ultimate L3 construct.

🆕 A user can remove a project's default compilerOptions while retaining the other properties of the tsconfig. This is because other properties like files and include are more universally useful than compilerOptions.

Pretty much, yes.

Here's everything TypeScriptProject does with TypescriptConfig (not counting things like gitignore):

  • creates the primary TypescriptConfig (unless configured not to), with the following merged on top of what is provided:
    • assigns it's name based on options + default
    • provides an include of the srcdir
    • provides compilerOptions.rootDir of the srcdir
    • provides compilerOptions.outDir of the libdir
    • 🛠️ merges defaults with provided options
    • exposes the tsconfig publicly
  • creates the dev TypescriptConfig (unless configured not to)
    • assigns it's name based on options + default
    • 🛠️ merges defaults with provided options AND tsconfig values
    • provides an include of the srcdir and the testdir
    • sets up exclude (node_modules, unsure why)
    • exposes the tsconfigDev publicly

I think we would want to keep everything as they are except the parts where the 🛠️ are.

Going back to your list:

(1) A project can provide default values for tsconfig

If AwsCdkTypeScriptApp were to, for whatever reason, want to provide a different set of default compilerOptions, it would either have to recreate what TypeScriptProject does now with TypescriptConfig or specifically set every possible setting (or just those in the TypeScriptProject default set plus any other it needs) when calling super(...).

(2) A user of this project can amend these default values, that is providing different and additional values to these defaults

Actually, I think if we could amend values (IOW, set them after super()), that would go a very long way to solve this and provide a mechanism for other things we've discussed above and elsewhere. (I know I read that slightly different than you meant it.)

Specifically, if we put TypescriptConfig.compilerOptions behind a getter and added a setter, and allowed it to be adjusted all the way up until synth time, then it could be inspected and manipulated as needed. That could form the basis for all of the other functionality. For example, one could make a component that takes a TypescriptConfig and intelligently adjusts the "target" while leaving everything else alone. Or applies an extends and removes the elements provided by that other config. Etc.

Sorry for the small novel.

Footnotes

  1. I did think "single file in the project" but that's too specific and inaccurate. For example, jest configuration actually can live in package.json, but I still consider the Jest component an L1, even when it doesn't write it's own file

@mrgrain
Copy link
Contributor

mrgrain commented Apr 2, 2024

This is a good write up. I'll need some time to work through this. But a quick note aside: I'm not convinced the L1, L2, L3 terminology is very useful for projen. Even with the AWS CDK it kind of falls apart with the L2s. But for projen it's even worse: TypescriptConfig uses JsonFile. So if JsonFile is an L1, TypescriptConfig can't be. But then what does this make Project TypeScriptProject and AwsCdkTypeScriptApp?

I don't have a better terminology at hand yet, just feel strongly that the Ls aren't cutting it. I also think it's important to differentiate strongly between Components and Projects. With the former being independent building blocks, and the latter being collections of Components; Patterns might be a fitting term for this.

Either way, it serves this discussion well enough, so I'm happy to go with this reasoning for now.

@giseburt
Copy link
Contributor Author

giseburt commented Apr 2, 2024

I agree. The L terminology feels more marketing than tech. The main reason I left it in was to express the composability. There are L1s, which are the raw file containers, then logic above that, then ever-larger patterns of coordinating that logic and other patterns. Escape hatches reach past the logic and patterns to adjust the raw data, but also duplicating effort to have to know and understand the file layout, intent, etc that was otherwise handled.

And that’s what was bugging me that I couldn’t get to directly but see now: not enough flexibility. I’m seeing this with the GitHub Workflows stuff too: too many things are set once at construction and can not be changed later, sometimes can’t even be read with reading the raw file data about to be written.

To see the full value of composition and patterns, we need to be able to adjust intelligently beyond component construction.

@mrgrain
Copy link
Contributor

mrgrain commented Apr 2, 2024

And that’s what was bugging me that I couldn’t get to directly but see now: not enough flexibility. [...] To see the full value of composition and patterns, we need to be able to adjust intelligently beyond component construction.

Yes and you are experiencing this because fundamentally the Construct Programming Model is build on the idea of correctness at construction time. Obviously we are way past this, but it has some nice properties as side effects. For example using a JSON RC file or passing values via the CLI.

@giseburt
Copy link
Contributor Author

giseburt commented Apr 3, 2024

I think we can ease composition for those elements used primarily inside the patterns by exposing manipulation methods and maintain correctness at construction time, and I agree it is a slippery slope.

For example, if we expose the ability to read and write TypescriptConfig.compilerOptions and then make components that adjust elements of those intelligently, such as changing the target, we can then expose a means to set the intended target in TypeScriptProject that can be both called later as well as set at construction time, preserving the ability to projen new a project with the intended target.

More importantly we don’t lose the value of the understanding of how tsconfig and targets work that we’re providing by making adjustments have to happen via escape hatches.

@giseburt
Copy link
Contributor Author

Two things:

  1. I am still working on this. I’ll have a draft PR soon. It’s think it’s actually simpler to implement once we discussed it all out. But I want to test carefully. Too often a prof of concept gets locked in.

  2. Yes and you are experiencing this because fundamentally the Construct Programming Model is build on the idea of correctness at construction time.

    Is there documentation on this somewhere? I’d love to read more about this. I thought the CDK Constructs docs maybe but I don’t see any mention of ideas like that mentioned, so I presume they’re enumerated somewhere.

@giseburt
Copy link
Contributor Author

Ok, I have made a PR that I think is a reasonable solution. I look forward to hearing feedback on this. I've been looking at it so long I worry I may have missed something important.

Here's the solution in a nutshell:

Phase 1:

  • Remove the private mergeTsconfigOptions fromsrc/typescript/typescript.ts
  • Add a TypescriptConfig.setCompilerOptions(newOptions: TypeScriptCompilerOptions, mergeMethod: TypeScriptSetCompilerOptionsMergeMethod) method allowing TypescriptConfig objects to be fully mutated (except for fileName) after construction
    • TypeScriptSetCompilerOptionsMergeMethod is an enum with three values: MERGE, MERGE_ALL, and OVERRIDE (descriptions from the new documentation):
      • MERGE will merge all compilerOptions with the existing options, including rootDir and outdir
      • MERGE_ALL will merge "safe" compilerOptions that are not managed by a project, such as rootDir and outdir
      • OVERRIDE will override "safe" compilerOptions that are not managed by a project, such as rootDir and outdir
  • Add a corresponding get compilerOptions() to TypescriptConfig so the current options can be inspected
  • Adjust as needed to make the above yield the same results in default cases (verified with the unmodified tests), then boost tests for the newly supported cases

Phase 2:

  • Add a new class TypescriptConfigPresets and a subclass ProjenClassicTypescriptConfigPresets that contains the current defaults from TypeScriptProject
  • Add function called getTypescriptConfigPresets that, given an enum, returns one of the built-in Presets object
  • Adjust TypeScriptProject to use the new presets, as well as new enum inputs tsconfigPresets and tsconfigDevPresets
    • Non-built-in presets can be used as well, but not using the enum-based selection process, and only applied after TypeScriptProject has been constructed
    • Presets can be selected for tsconfig and tsconfigDev separately
  • Also added a tsconfigDevExtendsTsconfig option to make it use extends in tsconfigDev to inherit the tsconfig options instead of copying them

Note about naming: I struggled with the term "presets," wanting to do names like ProjenClassicTypescriptConfigPreset instead of ProjenClassicTypescriptConfigPresets, but finally decided that "preset" implied a single value or even a collection of inseparable values, but instead it's a collection of settings that can individually be overridden later, thus "presets," even though in english that yields some plural confusion like tsconfigDevPresets being a single valued enum

I have only four presets currently:

export enum TypescriptConfigPresetsOptions {
  PROJEN_CLASSIC = "PROJEN_CLASSIC",
  NODE18 = "NODE18",
  NODE20 = "NODE20",
  STRICTEST = "STRICTEST",
}

PROJEN_CLASSIC is an exact copy of what used to be embedded in TypeScriptProject. There are a few oddities in there, such as strict: true and five other options set by strict also explicitly set. experimentalDecorators is also an odd option to enable. I bring these up since they're not set in the other presets. stripInternal is also not set in the other presets.

The other presets are almost exactly copied from tsconfig/bases (I removed two psychotic values from "strictest"). I feel like there's a ton of room for evolution of these presets, especially since they can be combined. For example, would we want to make the tsconfigPresets and tsconfigDevPresets into arrays of enums?

Anyway, please take a look over in #3535 and let me know what you think.

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.

3 participants