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

WIP: infra/configure editor setup #6

Closed
wants to merge 4 commits into from

Conversation

murattishkul
Copy link
Collaborator

@murattishkul murattishkul commented Mar 25, 2020

closes #3

Research recap:

[TsLint, EsLint] Folks at Palantir who once created tslint, to lint typescript code, decided to stop maintain it and turned their sight to eslint, referring to the fact that eslint is architecturally more efficient and has more rules and plugins that are maintained by huge open source community. Both typescript and tslint communities suggest to use @typescript-eslint to lint typescript code.

Eslint and Typescript compiler parse codebase to Abstract Syntax Tree data structure and then check for rules and types, that are specified in a list of plugins that add or extend rules in .eslintrc.json. @typescript-eslint suggest elegant way of linting by specifying @typesctipt-eslint/parser and plugin:@typescript-eslint/recommended in eslint config file. It will tell eslint to not use default parser and use @typesctipt-eslint/parser.

The latter, @typescript-eslint/recommended, has extensive list of rules that are, apparently by no means as large and thorough as eslint:recommend's list. However, I think that trade-off is justified for our relatively small project and overall @typesctipt-eslint is a good choice!

Checkout typescript-eslint monorepo for more info.

Configuration info set:
[Prettier] -

  • tab is 4 spaces,
  • semicolon and double quotes is a must.

[VsCode] -

  • our codebase is formatted on save with eslint and prettier --fix.
  • autosave is on after each focus change
  • use my favorite theme and icons extension

[Typescript] -

  • tsc compiles all .ts .d.ts .tsx files in src directory.
  • dist directory is specified as outDir (.js files after transpilation)

and [Babel] configuration is set by default.

Please reach out about any questions and things that I might miss in this short recap!

@murattishkul murattishkul self-assigned this Mar 25, 2020
@murattishkul murattishkul added the infra Infra/tooling-related enhancement label Mar 25, 2020
@murattishkul murattishkul added this to the project-setup milestone Mar 25, 2020
@murattishkul murattishkul changed the title Infra/configure editor setup WIP: Infra/configure editor setup Mar 25, 2020
@murattishkul murattishkul changed the base branch from master to develop March 25, 2020 22:10
@nugmanoff nugmanoff changed the title WIP: Infra/configure editor setup WIP: infra/configure editor setup Mar 26, 2020
@nugmanoff
Copy link
Collaborator

You may resolve WIP status now and please try to name your PR-s correspondingly based on your branch name. PR name that is defaulted by GitHub is not correct most of the time.

So your PR should be named: feature/configure_editor_setup and yours is Infra/configure editor setup (I made the first letter lowercase accidentally, sorry for that, couldn't resist :)

Why feature/ and not infra/. Basically I see that you followed the logic of prefixing your branch name with infra/ due to the fact that I labeled corresponding issue as so.
But due to the fact that we adhere to git-flow branching model and there is no specialized infra/ branch type - all branches that are not hotfix/ and release/ are feature/ by default.

Copy link
Collaborator

@nugmanoff nugmanoff left a comment

Choose a reason for hiding this comment

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

Also you can add extensions.json to .vscode/ folder and include VSCode plugins that we use and they will be shared & required to install as well.

"plugin:prettier/recommended"
],
"plugins": ["@typescript-eslint", "prettier"],
"env": { "node": true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure that we don't want to add es6 to environments?

"plugins": ["@typescript-eslint", "prettier"],
"env": { "node": true },
"parserOptions": {
"ecmaVersion": 5,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, I think it'd be better if we will opt for ES6

trailingComma: 'all',
singleQuote: false,
printWidth: 120,
tabWidth: 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also prefer this one most of the time, but what if we try making tabWidth: 2?
I think one of the benefits is the fact that we can use longer (thus more descriptive) variable names.

@@ -0,0 +1,10 @@
{
"editor.autoFixOnSave": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add vertical ruler rule so it will visually show us when we went too far to the right (and our code became unreadable).

{
"compilerOptions": {
/* Basic Options */
// "incremental": true, /* Enable incremental compilation */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you look for description of this property and maybe we should enable it - because incremental compilation is a great thing to have as far as I know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am just not sure how it work in TS

Choose a reason for hiding this comment

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

TypeScript 3.4 introduces a new flag called --incremental which tells TypeScript to save information about the project graph from the last compilation. The next time TypeScript is invoked with --incremental, it will use that information to detect the least costly way to type-check and emit changes to your project.

"compilerOptions": {
/* Basic Options */
// "incremental": true, /* Enable incremental compilation */
"target": "es5" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. */,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing with ES6

/* Basic Options */
// "incremental": true, /* Enable incremental compilation */
"target": "es5" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. */,
"module": "commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am missing a point a little bit here with module option listed here and there with various configurations. Can you please elaborate on this one? What is the difference between all them and which one should we use.
Or maybe you can send me one great article where I can look this all up altogether.

Choose a reason for hiding this comment

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

https://medium.com/@tommedema/typescript-confusion-tsconfig-json-module-moduleresolution-target-lib-explained-65db2c44b491

to be short according to ts documentation "module" depends on "target" such that
target === "ES3" or "ES5" ? "commonJS" : "ES6"

@nugmanoff
Copy link
Collaborator

Also when you write commit messages it is not necessary to add trailing . :)

@nugmanoff
Copy link
Collaborator

Basically you did a great job on this one! 👏🏻
Nice and thorough research. Just need to take it one step further by elaborating on details I mentioned & also I'd love to see a bit more info on how babel work with typescript (is there any issues or what should we know about the process transcompiling of TS, maybe there are some pitfalls). This one is optional though, but for the clarity I think you can include few links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra Infra/tooling-related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure editor setup + TSLint, Prettier
3 participants