-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
create-astro
: always create tsconfig.json
#4810
Conversation
Currently, we only make sure `tsconfig.json` exists when `strict` or `strictest` is selected. Both `default` & `optout` are intended to correspond to `base` -- and will do so for all [23 official templates](https://github.com/withastro/astro/tree/main/examples), but not necessarily for third-party templates. The [example command for installing a third-party template](https://github.com/withastro/astro/blob/a800bf7/packages/create-astro/README.md?plain=1#L31-L35) is (rather conveniently for the sake of this PR!) an example of a template without a `tsconfig.json` file, and installing it with the `default` ("Relaxed") Typescript option results in no `tsconfig.json` file, rather than a `tsconfig.json` file containing `{ "extends": "astro/tsconfigs/base" }` as would be expected. This PR addresses this scenario. It also explicitly sets the `tsconfig.json` file to `{ "extends": "astro/tsconfigs/base" }` when `default` (which I renamed to `base`, still presented to the user as "Relaxed") or `optout` is selected (`optout` has always printed a warning about the importance of `tsconfig.json` & `src/env.d.ts` but otherwise behaved identically to `default`). This is necessary in two scenarios: 1. When the `tsconfig.json` file was created by this script. 2. When it either didn't already include `"extends"`, or it extended a different config by default. For example, some third-party templates might default to `strict`, in which case I'm guessing we'd want to respect the user's choice and change that to `base`.
(without this it triggers many times)
🦋 Changeset detectedLatest commit: fe6ba76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -109,5 +109,5 @@ export default async function build(...args) { | |||
} | |||
|
|||
async function clean(outdir) { | |||
return del([`${outdir}/**`, `!${outdir}/**/*.d.ts`]); | |||
return deleteAsync([`${outdir}/**`, `!${outdir}/**/*.d.ts`]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is relevant here: https://github.com/sindresorhus/del#beware
Doesn't appear to be a breaking change from prior version: https://github.com/sindresorhus/del/tree/v6.1.1#beware
... So it should be behaving the same as it was before this PR.
(see if this fixes failing tests)
(since I'm about to trigger another CI run by pushing a commit, might as well try this too)
Typescript tests are slower than directory tests, but they are all usually less than 5000 ms. Less complexity, easier to maintain.
await wait(300); | ||
} | ||
if (args.dryRun) { | ||
ora().info(dim(`--dry-run enabled, skipping.`)); | ||
} else if (tsResponse.typescript) { | ||
if (tsResponse.typescript !== 'default') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff GitHub is showing here is unnecessarily complex, the only differences from here down are:
- Removed this line, and its matching closing
}
(former line 375) - Unindented former lines 347-374 one level (as you'd expect since a wrapping block was removed)
Alway write chosen config to `tsconfig.json`. | ||
|
||
- Before: Only when `strict` & `strictest` was selected | ||
- After: Also when `base` is selected (via "Relaxed" or "I prefer not to use TypeScript") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether users would be confused as to why they have an extra file in their project when they chose "no typescript"? I suppose they can just delete it later..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether users would be confused as to why they have an extra file in their project when they chose "no typescript"? I suppose they can just delete it later..
We're planning to update the message soon so it's more clear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing behavior (before this PR):
When you select "I prefer not to use Typescript", it prints this:
✔ How would you like to setup TypeScript? › I prefer not to use TypeScript
⚠ Astro ❤️ TypeScript!
Astro supports TypeScript inside of ".astro" component scripts, so
we still need to create some TypeScript-related files in your project.
You can safely ignore these files, but don't delete them!
(ex: tsconfig.json, src/env.d.ts)✔ TypeScript settings applied!
Also, before this PR, those files would always be there when using the official templates, they would only be missing if you used a third-party template that didn't include them.
// If the template doesn't have a tsconfig.json, let's add one instead | ||
fs.writeFileSync( | ||
templateTSConfigPath, | ||
stringify({ extends: `astro/tsconfigs/${tsResponse.typescript}` }, null, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if it's helpful linking to the docs for this in a comment as well?
https://docs.astro.build/en/guides/typescript/#setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a bad idea! I didn't change those lines (just dedented them), so doesn't necessarily need to be added in this PR, but might as well toss it in, feel free to add a commit for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR! The tests are notably lovely. I'm a bit thorn on if adding a tsconfig to third-party template is good or not, for the following reason:
- The template might be a monorepo, which shouldn't have a tsconfig.json with our extend in a lot of cases
- The template might be expecting specific tsconfig.json settings
Maybe for third-party templates that do have a tsconfig.json we could add another prompt option with something like "Use the template's settings".
HOWEVER, despite that, I think this is overall an improvement and since it's a fairly niche use case to use third party templates, we can change this in the future if it turns out to be a problem. Thank you again for this PR!
Excellent points, we could add a warning to the docs, or leave an issue open? |
Huh, weird, CI / Lint is failing on unrelated code... 🤔 |
Yeah, there's a bug in our CI pipeline that made this go unnoticed from another PR. No worries, it's unrelated to your changes! |
Changes
Alway write chosen config to
tsconfig.json
.strict
orstrictest
was selectedbase
is selected (via "Relaxed" or "I prefer not to use TypeScript")Testing
Added new
typescript-step.test.js
test file to test existing and new behavior related to the typescript prompt /--typescript
CLI argument).Docs
Existing
create-astro
docs don't mentiontsconfig.json
, so no changes are needed.Motivation
Before this PR:
If
base
:A) Do nothing: assumes a default
base
tsconfig.json
, which is the case for all 23 official templates, but not necessarily for third-party templates.If
strict
orstrictest
:A) Make sure
tsconfig.json
exists (create if missing).B) Write chosen
extends
totsconfig.json
(e.g.{ "extends": "astro/tsconfigs/strict" }
).After this PR:
base
is treated the same asstrict
orstrictest
.Template without
tsconfig.json
The example command for installing a third-party template -- rather conveniently for the sake of this PR -- uses a template without a
tsconfig.json
file, and installing it (prior to the changes in this PR) with thebase
Typescript option results in notsconfig.json
file, rather than atsconfig.json
file containing{ "extends": "astro/tsconfigs/base" }
as would be expected.Template with
tsconfig.json
not containing{ "extends": "astro/tsconfigs/base" }
If a third-party template doesn't use the
base
config, installing it (prior to the changes in this PR) with thebase
Typescript option results in no changes totsconfig.json
, rather than settingextends
toastro/tsconfigs/base
as would be expected.