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

Start of a recommended base #15

Closed
wants to merge 3 commits into from
Closed

Start of a recommended base #15

wants to merge 3 commits into from

Conversation

orta
Copy link
Member

@orta orta commented May 29, 2020

Opening this PR ahead a the TS design meeting for feedback

Copy link

@robpalme robpalme left a comment

Choose a reason for hiding this comment

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

Minor suggestions on nice defaults to propagate here:

  • declaration: true has the (possibly undocumented?) side-effect of catching more errors (increased strictness is good)
  • noEmitOnError: true helps prevent accidents (speaking from embarrassing bitter experience)
  • target: "esnext" & module: "esnext" will help steer codebases towards less transpilation (good for everyone)
  • useDefineForClassFields: true ensures checking & emit compatibility with engines

bases/recommended.json Outdated Show resolved Hide resolved
@styfle
Copy link
Contributor

styfle commented Jul 1, 2020

@robpalme Agreed! I think noEmitOnError: true is crucial for anyone starting a new TS project.

However, useDefineForClassFields: true doesn't seem to do anything when target: esnext.

It only seems relevant when target: es2020 or older, right?

@robpalme
Copy link

robpalme commented Jul 1, 2020

@styfle not quite.

useDefineForClassFields: true is still needed even with target: esnext. You can use the TypeScript playground to see the impact.

@orta
Copy link
Member Author

orta commented Jul 3, 2020

There's also microsoft/TypeScript#39354

@orta
Copy link
Member Author

orta commented Jul 3, 2020

We keep punting on this in design meetings, I'll get it seen soon enough

I agree with all of the feedback, updated the PR.

I'm not 100% enough on the target/module recommendation to put that in yet, will bring it up - I think I prefer it though, better to start at the top and let people choose to go back to earlier then the other way around.

"allowSyntheticDefaultImports": true,

/** Improves `tsc` launch speed in favour of some accuracy in your @types **/
"skipLibCheck": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs for skipLibCheck seem to be different than what your comment says here.

The docs say that this property will "Skip type checking of all declaration files (*.d.ts)."

https://www.typescriptlang.org/docs/handbook/compiler-options.html

Copy link

Choose a reason for hiding this comment

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

Yeah I can confirm this option turns off type checking of hand-written DTS files in your project, unfortunately. As a result I find it difficult to recommend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's a good point - I was thinking of types at the same time. We opted to recommend it for speed and that most dts files come from @types where we have tonnes of testing infra (and that most folks won't go fix these things upstream) - we'd probably fix the issues, others would get stuck

Copy link

Choose a reason for hiding this comment

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

That sounds like the common case - where the declarations files are for dependencies as opposed to defining types for files inside your project.

I know this isn't the right place to suggest it, but maybe the implementation of skipLibCheck could be updated to make this distinction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a bad call IMO

@orta
Copy link
Member Author

orta commented Jul 10, 2020

Managed to get this into a design review (we only do one a week, but given everyone is remote they're really filled, so might be switching to two a week) - general gist for this is that we think --init is basically the recommendation and any improvements I want there should happen there.

What that means for this PR is that I'm going to replace this with a @tsconfig/init which will always be permanently up to date (by extracting it from typescript in a nightly job and seeing if it's changed )

Then people know there is only 1 recommended by TS tsconfig. This probably means dropping some of the existing flags which I think are recommended.

I also wonder if something like tsconfig/node12 should be extending from @tsconfig/init and might need to build that infra too.

@orta
Copy link
Member Author

orta commented Aug 6, 2020

Alright, I've got this in with #25

It's bit kludgy with me manually editing all the other json files but otherwise I'll need to create the JSON files by string concatenation and I've already punted on this since May

@orta orta closed this Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants