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

chore: Remove display key from generated tsconfig #9225

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

Sensanaty
Copy link
Contributor

@Sensanaty Sensanaty commented Oct 7, 2024

Description

#9221

I noticed that the default create-turbo command would create tsconfigs that contained a display key, which isn't part of the TS schema spec.

I opened a discussion to ask about this, because it might've been a Turbo-specific feature, for example I thought it was maybe used to display the list of projects in the repo in a nice human-readable way, but according to Anthony it's not actually used for anything.

From a quick read through of the create-turbo code, what I gathered is that it basically just copies these files based on the hardcoded example repos, hence why I only modified the tsconfigs themselves. It's possible I missed something as this is my first time interacting with this codebase (and the word display is generic enough that it shows up in a billion places all over the code), so please do let me know if my assumption here was wrong.

Testing Instructions

  1. Create a new turborepo with create-turbo
  2. The example tsconfigs should no longer contain the display key.

@Sensanaty Sensanaty requested a review from a team as a code owner October 7, 2024 09:19
@turbo-orchestrator turbo-orchestrator bot added area: examples Improvements or additions to examples needs: triage New issues get this label. Remove it after triage owned-by: turborepo labels Oct 7, 2024
Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2024 1:19pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm
examples-gatsby-web ⬜️ Ignored (Inspect) Oct 7, 2024 1:19pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Oct 7, 2024 1:19pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm
examples-svelte-web ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 1:19pm

Copy link

vercel bot commented Oct 7, 2024

@Sensanaty is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@anthonyshew
Copy link
Contributor

As a mental note, I just verified that TypeScript doesn't verify extra keys at the top-level of tsconfig.json. I added a someRandomTestKey and everything passed successfully. However, adding a test key inside of compilerOptions does indeed error out.

That for me helps to confirm that this was in the initial template from 3 years ago, kept getting copy-pasted and, since it never hurt anything, no one questioned it.

Copy link
Contributor

@anthonyshew anthonyshew left a comment

Choose a reason for hiding this comment

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

Thanks! Looks right to me.

Out of an abundance of caution, I've asked the rest of the team if anyone has any secret sage wisdom about why this key might be in these, just in case there's something I'm not realizing. I don't think this will be the case, but I'll come back and merge later today assuming I don't hear any objections.

@ijjk
Copy link
Member

ijjk commented Oct 7, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8f9ac58

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

1 similar comment
@ijjk
Copy link
Member

ijjk commented Oct 7, 2024

Allow CI Workflow Run

  • approve CI run for commit: 8f9ac58

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link
Contributor

@anthonyshew anthonyshew left a comment

Choose a reason for hiding this comment

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

Talked to colleagues and they're good with this, too!

The CI failures here are not related to these changes. I will fix in a separate PR.

Thank you for your contribution! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Improvements or additions to examples needs: triage New issues get this label. Remove it after triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants