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

feat(create-astro): add astro checks to build script when user choose TypeScript #8853

Merged
merged 13 commits into from
Oct 23, 2023

Conversation

rayriffy
Copy link
Contributor

@rayriffy rayriffy commented Oct 17, 2023

Changes

  • If user choose TypeScript strictness to strictest, the create-astro command will also add astro checks to build script
  • Additionally, @astrojs/check and typescript will be installed if user previously opt-in to install dependencies
  • Internally, some parts of typescript.ts uses both async/sync of node:fs. I also did some refactoring to be all async

fixes #7031

Testing

  • Writing test to make sure that build script package.json is actually changed

Docs

Maybe need to add extra docs to TypeScript to explain strictest behavior when use pnpm create astro

@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2023

🦋 Changeset detected

Latest commit: ff7d695

The changes in this PR will be included in the next version bump.

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

@github-actions github-actions bot added the pkg: create-astro Related to the `create-astro` package (scope) label Oct 17, 2023
@rayriffy rayriffy changed the title feat: add astro checks on strictest feat(create-astro): add astro checks to build script on strictest Oct 17, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

This is great, thanks so much for contributing!

Just wondering about always applying this if the user sets up TypeScript. I usually go with strict but I'd still like this to be setup for me.

packages/create-astro/src/actions/typescript.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 21, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks so much for the PR!

.changeset/ninety-onions-bow.md Outdated Show resolved Hide resolved
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just a quick suggestion from docs re: the wording here! See whether you think the extra context is helpful!

'create-astro': minor
---

Automatically setup `astro check` command when configuring TypeScript
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Automatically setup `astro check` command when configuring TypeScript
Automatically installs the required dependencies to run the `astro check` command when `strict` or `strictest`
TypeScript mode is selected.

Since running astro check requires both @astrojs/check and typescript, maybe makes sense to say "required dependencies" (plural) here, because these will be added to your project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do agree. But I have to adjust some wording at the end because astro check will be applied to all TypeScript options (relaxed, strict, strictest).

Let me know if this change looks good to you.

Copy link
Member

Choose a reason for hiding this comment

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

I commented above to clarify that this happens on relaxed (which seems now far removed from the PR title itself, and maybe installs a dependency for people who won't really use it?) But, wording should be fine.

@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Oct 23, 2023
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Just a comment, but feature looks fine to me. I do feel like we'll get support threads of people complaining that their project doesn't build with various errors, but meh, it's okay, people should and will learn.

@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 23, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Left a comment to clarify with an optional suggestion, but also fine as is!

.changeset/ninety-onions-bow.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Oct 23, 2023
@rayriffy rayriffy changed the title feat(create-astro): add astro checks to build script on strictest feat(create-astro): add astro checks to build script when user choose TypeScript Oct 23, 2023
.changeset/ninety-onions-bow.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 23, 2023
@natemoo-re natemoo-re removed the pr: docs A PR that includes documentation for review label Oct 23, 2023
@natemoo-re natemoo-re merged commit ce807a2 into withastro:main Oct 23, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 23, 2023
natemoo-re added a commit that referenced this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: create-astro Related to the `create-astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript starter template doesn't include required commands
4 participants