-
Notifications
You must be signed in to change notification settings - Fork 5
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
Setup TypeScript and adjusting other configurations accordingly #10
Conversation
Because the project uses the new JSX transform from React 17, React doesn't have to be imported in each file. This new shared config turns off ESLint rules that warns about missing React imports.
Expensify/App#22703 is hold on this so I gonna go ahead and merge 🙇 |
@hayata-suenaga looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
I don't know how melvin determines if tests passed or not. We should check on this |
Build failed upon merge because of the misconfigured TypeScript setup. Making a PR to fix this. Failed workflow: |
cc: @tgolen @marcaaron @neil-marcellini
Related Issues
GH_LINK: #7
Manual Tests
No test
Linked PRs
N/A
Details
I listed out the changes I made in this PR and explanation of each of the changes.
Package entry point
index.js
entry point for all platforms (we should import files with different platform extensions INSIDE index.js). Just for testing purpose for now.Prettier configuration
printWidth
. It's currently set to190
, which is unusually long. Prettier recommend against setting this value greater than 80.. The default, which is used when this option is not specified, is 80.Babel configuration
.prettierignore
GH workflow for linting
Because we're now running TypeScript as part of this workflow,
linting
doesn't make sense anymore.actions/checkout
andactions/setup-node
actions/cache
actions/setup-node
hascache
option that can be used to cache node and dependenciesJust running
Prettier
errors if there is any Prettier violations. No need to check-diff
Removed
eslint-config-expensify
I know this might be a little bit controversial change. There are five motivations.
This is the list of the rules that are turned off in App in the TypeScript setup PR.
In the eslint config file, we had to do a lot adjustments.
prefer-destructuring
)eslint-config-prettier
so they have no effect at allConsidering the above points,
eslint-config-expensify
can be distilled with four rules that need to be specified in this repo'seslintrc.js
four rules in eslintrc.js
The main changes that result from the elimination of eslint-config-expensify are the following.
rulesdir/no-negated-variables
rulesdir/prefer-early-return
These won't be enforced anymore as we're removing
eslint-config-expensify
. We can move these custom rules to the repo if necessary.I went through each rule in eslint-config-expensify and evaluated each of them. You can see my memo for each of them below.
react.js
react/no-multi-comp
This rule is
off
by default (airbnb also turns this off) so there is no reason to explicitly turn this offreact/react-in-jsx-scope
This is turned off when extending
plugin:react/jsx-runtime
react/jsx-indent-props
,react/jsx-indent
,react/jsx-no-undef
These rules are enforced by
Prettier
react/prefer-es6-class
We don't want the usage of
createReactClass
, so the rule is irrelevantreact/prefer-stateless-function
We uses functional components for everything now, so the rule is irrelevant
react/prop-types
This rule is already turned on with
recommended
configreact/jsx-props-no-multi-spaces
Enforced by Prettier
react/no-find-dom-node
This rule is already turned on with
recommended
configreact/forbid-prop-types
This rule is irrelevant because of TypeScript
react/no-string-refs
This rule is already turned on with
recommended
config'react/jsx-filename-extension': [1, {extensions: ['.js']}]
We decided to start using
.jsx
so this option conflicts with that decisionreact/destructuring-assignment
This one is
off
now but because we decided to start using de-structuring, we should turn this on.react/function-component-definition
This one makes sense. Added this in
eslint.config.json
of this repo.react/no-unsafe
Because we're no longer using class components, this is irrelevant
eslint-config-expensify
display-name-property.js
This rule is already covered by
react/display-name
which is included inplugin:react/recommended
no-api-in-views.js
This one checks any usage of
API
orDeprecatedAPI
, which is not relevant outsideApp
no-api-side-effects-method.js
This rule just flags any usage of
makeRequestWithSideEffects
, which is not relevant outisdeApp
no-call-actions-from-actions.js
This rule looks for imports from files under
actions
directory, which makes this rule irrelevant outsideApp
no-inline-named-export.js
This one is a general rule that prevents inline exports. However, it's common for library code to contain line exports. By the fact that there is no existing ESLint to enforce this, there doesn't seem any widely recognized reason to ban them.
no-multiple-api-calls.js
This checks that only one
API
is used per function, which isApp
specificno-negated-variables.js
This checks that certain negation words are not used in variable or function names. This is a general rule but the usage of
NOTABLE_EXCEPTIONS
makes this rule specific. Also, adding an exception every time an exception needs to made is cumbersome to maintain.no-thenable-actions-in-views.js
This one prevents the usage of any functions that returns a promise from the
actions
directory. This one is alsoApp
specific.no-useless-compose.js
I believe this rule checks if the
compose
that combines HOCs is called with a single argument. This is alsoApp
specific.onyx-props-must-have-default.js
Just from the name, this is
Onyx
specific.prefer-actions-set-data.js
This rule checks if certain methods on
Onyx
are used outside files with certain names. This is alsoApp
specific.prefer-early-return.js
This rule is the most generic one and might be useful.
This rule encourages the coding standard like this
Instead of
prefer-import-module-contents.js
This is the rule that we decided to turn off for TypeScript projects.
This rule prevents the following style of import statements
prefer-localization.js
This rule checks if date is localized in "action" files. Because this only checks for action files, this rule is also "App" specific.
prefer-onyx-connect-in-libs.js
From the rule name, this one is
App
andOnyx
specific.prefer-str-method.js
This rule checks if string methods from
expensify-common
can be used. The library is not migrated to TypeScript. Also, we removed this library inOnyx
recently. Rather than using a few methods fromexpensify-common
which we have to maintain, if necessary, we can just define utility methods in the repo itself.prefer-underscore-method.js
We decided to not to use
underscore
anymore inApp
. We should adopt the same strategy in all repos.style.js
All rules in this file except the following are turned off when extending
prettier
, so they're irrelevant.import/no-dynamic-require
This one is turned on by
airbnb
and turned off by expensify ESLint rule. However, unless there is a specific need, we should keep thison
. We probably don't need use expression insiderequire
statements (and we probably gonna use ES6 import/export syntax most of the time anyway).import/no-extraneous-dependencies
This rule is turned on by
airbnb
. However, there is no reason to turn this off. The options specified inairbnb
also works for this repository.import/no-unresolved
This rule is on if
import/recommended
is extended. There seems to no reason to turn this off. We want to be warned if we use any module that doesn't exist.jsx-a11y/click-events-have-key-events
andjsx-a11y/label-has-for
These accessibility rules are turned off but we can turn off these rules only once they're determined not needed. Most of the accessibility props will be pass though when creating libraries. Keeping these rules will prevent us from forgetting providing these accessibility props in custom components and passing these to native components.
es6.js
prefer-template
This is already turned on by
airbnb
. No need to explicitly turn this on.prefer-destructuring
We decided we can use de-structuring now. No need to turn this off. The default option set in
airbnb
seems reasonable/jsx-a11y/label-has-associated-control
I don't know why
jsx-a11y
are spread across multiple files. We can adjust these settings as we write the library. Most of the accessibility props would be pass through. So probably no need for adjustments.arrow-parens
The option specified for this rule is different from
airbnb
, so I'm adding this option to this repo.@lwc/lwc/no-async-await
This rule can be replaced with
no-restricted-syntax
rule with selectors specified. Add a rule for this in theeslintrc.js
of this repo.es/no-nullish-coalescing-operators
,es/no-optional-chaining
These are features that has good compatibility with TypeScript. These features should be allowed (we turned these rules off in App)
no-invalid-this
This is a good rule.
airbnb
turns this off so turning back this on in this repo.