-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Proposed tsconfig.json
changes
#3019
Comments
I almost forgot: I also have to change esbuild's decorator support to check the |
I have yet to read the whole thing and think about it yet (probably Monday when I'm back in work-mode), but regarding:
I find this sorta weird because multi-tsconfig is sort of how TypeScript works... if you're using build mode and project references, or even if you have two tsconfigs that would theoretically include different files. I think that's how most large projects are structured. For example, now that TypeScript itself is bundled by source through esbuild, we do have many tsconfigs that could apply different settings for their respective folders. I don't think I have anything set in those that would actually matter, but, I wouldn't put it past someone to have some packages of a monorepo at one target and one at another, or some part of their codebase that uses one JSX factory style but another somewhere else. But, these are really silly and I expect nobody to do them either. I'd have to think harder about this, though. Clearly, there's a lot of options listed above, and I've definitely read that |
Thanks very much for weighing in. Sorry for writing so much text about this 😅 I'm not sure what to do because some Another problem seems to be that people have random I could create some inconsistent case-by-case rules for which individual fields come from which Here's another proposal that would be a smaller change:
|
Hi @evanw, thanks for this write-up. // bar.android.ts (Android specific file)
export default (x: string) => string;
// bar.ios.ts (iOS specific file)
export default null;
// bar.ts (fallback, typically used to target web)
export default (x: string) => string;
// foo.ts
import bar from "./bar";
bar("x"); Without With esbuild evaluating this field per package is where we get into trouble. esbuild can pick up files for other platforms than the user intended. A dependency could have IMO, Update: I've just learned that |
Re useDefineForClassFields: it is indeed very subtle, but it's unlikely to cause widespread breakage for a couple of reasons:
|
I brought this up in a TypeScript design meeting to try and get extra eyes on this. I think there are a few on the team who are going to be looking at this harder so we can talk about it again next week with some more context and opinions (as clearly this is nuanced enough to require some thought). I assume that you're not going to implement these changes "soon", right, given the break-y-ness? (I'm probably going to hold off on giving my thoughts until I've put them together as I've been busy with other stuff!) |
Thanks for your replies. I can implement this either soon or not soon, depending on people's input. From this it sounds like I should wait for people to think about it more. I'd like to make the change reasonably soon though because it's responsible for a cluster of tsconfig-related issues that have been broken for a long time. |
My raw notes are at https://gist.github.com/jakebailey/1e4df8b3a33d2da7b0db4ae39861afd8, but here's the general feelings I have with more context. This is going to be a large brain dump, so I'm totally happy to break this down in follow ups. Also, these are generally just "my opinions", other team members may also provide their own views, but what I have written up below didn't seem obviously wrong to anyone who read it, so I'm sending it over! Generally, I think that esbuild's behavior should be guided by "what would happen if a user ran Unfortunately (for the you and the implementation), I think there are too many gotchas for not supporting multiple So, as a long list, my personal recommendations for the listed options are roughly:
This is a bit of a condensed brain dump, so I'm totally happy to spitball and discuss the above. Alternatively, there is another option... which is to completely ignore
But, I don't know if that's going to be the most user-friendly option either. |
Thank you very much again for such thoughtful and detailed replies. I really appreciate it. My replies
I'm assuming you mean "if a user ran
I suspect there are some people that are doing this intentionally (via Hyrum's Law). For example, I could see it being useful to ensure that I'm wondering if a good change here could be to shift all TypeScript file extensions to last in the "resolve extensions" order when inside Regarding monorepos with symlinks: Like node, esbuild defaults to using the real path of an input file as its identity. So if there are symlinks in
Specifically: esbuild will check the value of if ('useDefineForClassFields' in tsconfig)
esbuild.useDefineForClassFields = tsconfig.useDefineForClassFields;
else if ('target' in tsconfig)
esbuild.useDefineForClassFields = greaterThanOrEqual(tsconfig.target, 'ES2022');
else
esbuild.useDefineForClassFields = true; // esbuild will now be defaulting to true here instead of false
Yes, I plan to add support for if (tsconfig.verbatimModuleSyntax === true)
esbuild.unusedImportFlags |= UnusedImportFlags.KeepPath | UnusedImportFlags.KeepValues;
if (tsconfig.preserveValueImports === true)
esbuild.unusedImportFlags |= UnusedImportFlags.KeepValues;
if (tsconfig.importsNotUsedAsValues === 'preserve' || tsconfig.importsNotUsedAsValues === 'error')
esbuild.unusedImportFlags |= UnusedImportFlags.KeepPath;
For the same reason as TypeScript I imagine. People may be doing a custom JSX transform. For example, some people apparently remove certain React properties for tests. SolidJS also apparently has its own JSX transform that compiles to template strings with DOM manipulation. Updated proposal
Note that this list does not contain a rule saying "ignore all I agree that esbuild needs to be more flexible with setting these values not using I should probably also mention that esbuild has a |
These tweaks all seem good to me. Some commentary:
No, if you have more than one tsconfig, I think it's very possible (and probably more common) to use a single invocation by using project references and I think some monorepo solutions don't use project references, but then instead rely on custom orchestration of multiple
This to me feels like it's going to end up sometimes producing surprising behavior. Here are some example "spooky" cases that I had thought up:
For the latter case, it at least will probably cause The saving grace is that these sorts of things are exceedinly rare. Most people use something like Also, this is generally not a problem unique So all in all, I'm not strongly attached to maintaining your current behavior for |
Normally, I'd say this is very scary! But, I suppose since this is bundled, it's probably fine? Assuming that external const enums don't make it into the bundle if that package is marked as external? TS itself uses const enums internally, but then there's a build step that drops |
Just another data point for you. We are currently stuck on version |
Not directly related to In both esbuild-loader and tsx, I use a file matcher to test if a given file falls within the scope of the provided However, this would introduce a breaking change (hence why I thought discussing it here would be relevant). Additionally, it would require providing the path of the |
Ok, here are the
This fixes most of the open issues related to This doesn't include the change to omit |
Sorry it’s a bit late, arrived via release notes (congrats!) Would it be possible to make .js > .ts configurable? Perhaps by respecting the order of resolveExtensions? Asking because there’s still a fair amount of packages that do some wonky transpilation, esp re decorators. Including ts sibling files allows a consumer to transpile all TS uniformly using their esbuild installation & config. Additionally, .js can be lowered to an unwanted target, JSX rewritten to , etc |
Do you have any examples for this? The decorator case is more or less the exact case why I wanted to ensure that |
Really just all of transpiled history. The number of implementations that have come and gone over the years to deal with decorators… The point is do with unwanted output settings. Whether the author is delivering decorators (or anything) as ES3/5/lower-than-I-want, I’m now stuck with it. If they offer the raw TS source, I can transpile using whatever settings I want. not looking to change the default — just make an escape hatch |
Hey :), can this warning the tsconfig.json target is used in eslint for a example, so its not possible to remove i from the config... |
Warnings can be changed with a log override: https://esbuild.github.io/api/#log-override. Each warning has the category name in square brackets at the end, which in this case is |
For the record (no action needed): Here's an example of someone wanting to use |
Here are all of the currently-open changes related to
tsconfig.json
files:esnext
target doesn't includeuseDefineForClassFields: true
, unlike TypeScript #2993target
in tsconfig doesn't affect esbuild's runtime library #2628--target=esnext --tsconfig-raw='{"compilerOptions": { "target": "es2021" }}'
will defaultuseDefineForClassFields
totrue
#2584[email protected]
usescompilerOptions.baseUrl
when resolving transitive dependencies #2481moduleSuffixes
per package instead of per bundle #2395Common themes are: problems when
tsconfig.json
overrides esbuild's settings, problems whentsconfig.json
doesn't override esbuild's settings, and problems when esbuild's interpretation oftsconfig.json
affects code innode_modules
.This issue documents my thoughts about how to solve these issues.
Background
The typical workflow with the TypeScript compiler is that each run of
tsc
converts TypeScript files to JavaScript one-at-a-time. Then you either run the JavaScript directly, or bundle it and then run that. Each execution oftsc
has at most one roottsconfig.json
file. So if your TypeScript project is split into two directories (each with its owntsconfig.json
file), you'd have to runtsc
at least twice.For convenience, esbuild added support for loading some settings from
tsconfig.json
files. It was originally justbaseUrl
, thenpaths
, thentarget
, and more. Here's the full list:However, the way that esbuild does it is different than
tsc
. Each source file is affected by thetsconfig.json
file in the nearest enclosing directory for that source file. This lets you use esbuild to bundle a TypeScript project that is split into two directories (each with its owntsconfig.json
file) in a single step instead of having to run esbuild at least twice like you have to do withtsc
(which would be slower and potentially generate worse code).This approach that esbuild took initially made sense for
baseUrl
andpaths
, but doesn't really make sense fortarget
, and is iffy for some of the other settings as well. The reason why this doesn't make sense fortarget
is that you probably wouldn't want part of your bundle to be lowered toES5
while another part of your bundle to useESNext
syntax.What to do about this
At this point esbuild is widely-used, so any change to this is going to break someone. But this needs to be changed for these issues to be fixed. So this decision is also about deciding which workflows to break.
Arguably supporting multiple
tsconfig.json
files at once was a mistake becausetsc
doesn't work like that. So at the moment I'm thinking about removing support for doing this. That would fix a lot of these issues, and would likely also help align how esbuild works with people's existing mental model for how TypeScript works. But I'm sure this will break someone so I'd only do it in a breaking change release, and I'm also talking about doing it in this issue ahead of time.Another issue is that TypeScript only applies
tsconfig.json
to TypeScript files by default, while esbuild processes both TypeScript and JavaScript files by default. So for example"target": "ES6"
lowers code in both.ts
and.js
files to JavaScript,"jsx": "preserve"
applies to both.tsx
and.jsx
, andbaseUrl
andpaths
are interpreted in both.ts
and.js
files (including those innode_modules
).Here's one proposal to address these outstanding issues:
Change esbuild to only use a single
tsconfig.json
fileMost
tsconfig.json
settings would no longer apply to file paths containing anode_modules
path segment. Also, mosttsconfig.json
settings would no longer apply to JavaScript files unless you use"allowJs": true
orjsconfig.json
. Specifically:jsx
target
importsNotUsedAsValues
preserveValueImports
useDefineForClassFields
node_modules
alwaysStrict
baseUrl
jsxFactory
jsxFragmentFactory
jsxImportSource
paths
allowJs
is enabled), except it's not applied when bundling is enabled and the TypeScript file is innode_modules
Remove esbuild's support for TypeScript's
moduleSuffixes
setting (this was requested by someone)Change esbuild to default
useDefineForClassFields
to true (i.e. use JavaScript semantics for TypeScript class fields by default). This is worth calling out becausetsc
defaultsuseDefineForClassFields
to false, so this means that esbuild would compile class fields differently than TypeScript by default. The underlying reason is that esbuild's target defaults toesnext
while TypeScript's target defaults to ES5, and TypeScript also has some weird behavior where class fields behave differently depending on the target. I currently have esbuild emulating TypeScript's weird behavior when you explicitly set a target, but if you leave everything at their default values, esbuild currently defaultsuseDefineForClassFields
to false liketsc
. Changing esbuild to defaultuseDefineForClassFields
to true has been requested several times, so I'm planning to do it, but it's very subtle and I'm guessing it will cause lots of unfortunate breakage. Much more info and history about this is here: esbuild's defaultesnext
target doesn't includeuseDefineForClassFields: true
, unlike TypeScript #2993 (comment).I also need to add support for TypeScript 5.0's new
verbatimModuleSyntax
setting andextends
multiple inheritance, which I could do along with these othertsconfig.json
changes.The text was updated successfully, but these errors were encountered: