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.

Sign up for GitHub

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

Jump to bottom

Take "target" from base tsconfig when present #55

Closed
wants to merge 1 commit into from
Closed

Take "target" from base tsconfig when present #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
# 1.13

- Take `target` from `tsconfig.json` if present, rather than
hard-coding in the `build.json` config.

# 1.12

- Respect `package.json` type field if set to `"commonjs"`
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ following caveats:
- `outDir` - will be overridden based on build, best omitted
- `rootDir` - will be set to `./src` in the build, can only
cause annoying errors otherwise.
- `target` - will be set to `es2022`
- `target` - will be set to `es2022` if not specified
- `module` - will be set to `NodeNext`
- `moduleResolution` - will be set to `NodeNext`

Expand Down
12 changes: 8 additions & 4 deletions src/tsconfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import * as console from './console.js'
import config from './config.js'
import polyfills from './polyfills.js'
import preventVerbatimModuleSyntax from './prevent-verbatim-module-syntax.js'
import readTypescriptConfig from './read-typescript-config.js'

const {
dialects = ['esm', 'commonjs'],
Expand Down Expand Up @@ -43,18 +44,21 @@ const recommended: Record<string, any> = {
},
}

const build: Record<string, any> = {
const build = (): Record<string, any> => ({
extends:
config.project === undefined
? '../tsconfig.json'
: join('..', config.project),
compilerOptions: {
target:
readTypescriptConfig().options.target === undefined
? 'es2022'
: undefined,
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
: undefined,
target: readTypeScriptConfig().options.target ?? 'es2022'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we're doing here is subtly different from that:

  • if the target is undefined in the tsconfig.json, then we set es2022
  • if the target is defined, then we don't set it at all, so that it inherits from the base tsconfig.

With your suggestion we are instead setting the target in build.json to be the same as in the base tsconfig -- we could do that, but then we'd need to pull in a bit more logic from TS to stringify the value again (the parsed value is actually a ScriptTarget, which TS defines as a numeric enum).

Copy link
Owner

Choose a reason for hiding this comment

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

Oic, that makes sense. Thanks for the clarification.

rootDir: '../src',
target: 'es2022',
module: 'nodenext',
moduleResolution: 'nodenext',
},
}
})

const commonjs = (dialect: string): Record<string, any> => {
const exclude = [...relativeExclude, '../src/**/*.mts']
Expand Down Expand Up @@ -112,7 +116,7 @@ if (config.project === undefined && !existsSync('tsconfig.json')) {
for (const f of readdirSync('.tshy')) {
unlinkSync(resolve('.tshy', f))
}
writeConfig('build', build)
writeConfig('build', build())
if (dialects.includes('commonjs')) {
writeConfig('commonjs', commonjs('cjs'))
for (const d of commonjsDialects) {
Expand Down
1 change: 0 additions & 1 deletion tap-snapshots/test/tsconfig.ts.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ Object {
"module": "nodenext",
"moduleResolution": "nodenext",
"rootDir": "../src",
"target": "es2022",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have tests for whether we preserve the target and when we don't?

Copy link
Contributor Author

@timovv timovv Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The existing test case actually covers that case already, since it

  • Generates everything from scratch, including the recommended tsconfig.json (which sets target to es2022)
  • Then it makes a snapshot -- this is the snapshot you're commenting on, and the target field is no longer present, as expected with the new behavior
  • Then the tsconfig.json is overwritten with a custom configuration, which omits the target field
  • Finally it takes another snapshot, which is there is no diff for since target is still being set to es2022 as it used to be

},
"extends": "../tsconfig.json",
}
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/basic-custom-project/.tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.custom.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/basic-imports-only-deps/.tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/basic/.tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/imports-with-star/.tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
1 change: 0 additions & 1 deletion test/fixtures/imports/.tshy/build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": "../tsconfig.json",
"compilerOptions": {
"rootDir": "../src",
"target": "es2022",
"module": "nodenext",
"moduleResolution": "nodenext"
}
Expand Down
Loading