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

Setup TypeScript and adjusting other configurations accordingly #10

Merged
merged 34 commits into from
Aug 2, 2023

Conversation

hayata-suenaga
Copy link
Contributor

@hayata-suenaga hayata-suenaga commented Jul 31, 2023

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

  • Added three entry points for different platforms. But probably we shouldn't do this and have a single index.js entry point for all platforms (we should import files with different platform extensions INSIDE index.js). Just for testing purpose for now.
  "main": "./dist/index.js",
  "react-native": "./dist/index.js",
  "browser": "./dist/index.web.js",

Prettier configuration

Babel configuration

  • We don't have to differentiate web and mobile when transpiling libraries. It's the user of the library's job to use a bundler (ex. Metro or Webpack) to bundle builds for different platforms
  • Because we don't need to check platform, we can use a single json file format.

.prettierignore

  • At least for GH workflow for linting, there was no issue with the content of this file commented out. We can uncomment once an issue arises.

GH workflow for linting

  • Rename the file and workflow names
    Because we're now running TypeScript as part of this workflow, linting doesn't make sense anymore.
  • Update GH actions versions: actions/checkout and actions/setup-node
  • Removed the step that runs actions/cache
    actions/setup-node has cache option that can be used to cache node and dependencies
  • Simplified the step to turn Prettier
    Just running Prettier errors if there is any Prettier violations. No need to check -diff
  • Added a step to run TypeScript checks

Removed eslint-config-expensify

I know this might be a little bit controversial change. There are five motivations.

  • Most of the custom ESLint rules are specific to App (for example, rules that enforce certain methods in App to be used in a certain files in certain ways).
  • Some custom ESLint rules don't work well with TypeScript code and they need to manually turned off anyway
    This is the list of the rules that are turned off in App in the TypeScript setup PR.
    • 'rulesdir/onyx-props-must-have-default': 'off'
    • 'rulesdir/prefer-underscore-method': 'off',
    • 'rulesdir/prefer-import-module-contents': 'off',
  • Some rules and rule options don't make sense in the context of a TypeScript project
    In the eslint config file, we had to do a lot adjustments.
  • Some rules are out of sync with the current style guideline (ex. prefer-destructuring)
  • All style related rules are turned off by eslint-config-prettier so they have no effect at all

Considering the above points, eslint-config-expensify can be distilled with four rules that need to be specified in this repo's eslintrc.js

four rules in eslintrc.js
rules: {
        'arrow-parens': [
            'error',
            'as-needed',
            {
                requireForBlockBody: true,
            },
        ],
        'no-invalid-this': 'error',
        'react/function-component-definition': [
            'error',
            {
                namedComponents: 'function-declaration',
                unnamedComponents: 'arrow-function',
            },
        ],
        'no-restricted-syntax': [
            'error',
            // The following four selectors prevent the usage of async/await
            {
                selector: 'AwaitExpression',
                message: 'async/await is not allowed',
            },
            {
                selector: 'FunctionDeclaration[async=true]',
                message: 'async functions are not allowed',
            },
            {
                selector: 'FunctionExpression[async=true]',
                message: 'async functions are not allowed',
            },
            {
                selector: 'ArrowFunctionExpression[async=true]',
                message: 'async functions are not allowed',
            },
            {
                selector: 'MethodDefinition[async=true]',
                message: 'async methods are not allowed',
            },
        ],
    },

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 off

  • react/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 irrelevant

  • react/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 config

  • react/jsx-props-no-multi-spaces
    Enforced by Prettier

  • react/no-find-dom-node
    This rule is already turned on with recommended config

  • react/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 decision

  • react/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 in plugin:react/recommended

  • no-api-in-views.js
    This one checks any usage of API or DeprecatedAPI, which is not relevant outside App

  • no-api-side-effects-method.js
    This rule just flags any usage of makeRequestWithSideEffects, which is not relevant outisde App

  • no-call-actions-from-actions.js
    This rule looks for imports from files under actions directory, which makes this rule irrelevant outside App

  • 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 is App specific

  • no-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 also App specific.

  • no-useless-compose.js
    I believe this rule checks if the compose that combines HOCs is called with a single argument. This is also App 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 also App specific.

  • prefer-early-return.js
    This rule is the most generic one and might be useful.

    This rule encourages the coding standard like this

    function myFunction(condition) {
      if (!condition) {
          return;
      }
         console.log('Condition is true');
      }

    Instead of

    function myFunction(condition) {
      if (condition) {
          console.log('Condition is true');
      }
     }
  • 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

    import { myFunction, myComponent } from './localFile';
  • 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 and Onyx 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 in Onyx recently. Rather than using a few methods from expensify-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 in App. 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 this on. We probably don't need use expression inside require 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 in airbnb 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 and jsx-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 the eslintrc.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.

@hayata-suenaga hayata-suenaga self-assigned this Jul 31, 2023
@hayata-suenaga hayata-suenaga requested a review from a team August 1, 2023 23:36
@melvin-bot melvin-bot bot requested review from marcochavezf and removed request for a team August 1, 2023 23:36
@hayata-suenaga hayata-suenaga marked this pull request as ready for review August 1, 2023 23:37
@hayata-suenaga hayata-suenaga changed the title Hayata update config Setup TypeScript and adjusting other configurations accordingly Aug 2, 2023
src/index.web.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@hayata-suenaga
Copy link
Contributor Author

Expensify/App#22703 is hold on this so I gonna go ahead and merge 🙇

@hayata-suenaga hayata-suenaga merged commit b54b62f into main Aug 2, 2023
2 checks passed
@melvin-bot
Copy link

melvin-bot bot commented Aug 2, 2023

@hayata-suenaga looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@hayata-suenaga
Copy link
Contributor Author

hayata-suenaga commented Aug 2, 2023

@hayata-suenaga looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

I don't know how melvin determines if tests passed or not. We should check on this

@hayata-suenaga
Copy link
Contributor Author

hayata-suenaga commented Aug 2, 2023

also, PR merge doesn't automatically trigger delete of the branch. need ring0 ? permission to change repo setting. will create an issue for this

Screenshot 2023-08-02 at 3 30 12 PM Screenshot 2023-08-02 at 3 31 19 PM

@hayata-suenaga hayata-suenaga deleted the hayata-update-config branch August 2, 2023 22:31
@hayata-suenaga
Copy link
Contributor Author

hayata-suenaga commented Aug 2, 2023

Build failed upon merge because of the misconfigured TypeScript setup. Making a PR to fix this.

Failed workflow: Publish package to npmjs

Screenshot 2023-08-02 at 3 38 01 PM

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.

2 participants