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

feat: add TypeScript support #106

Merged
merged 7 commits into from Apr 16, 2019
Merged

feat: add TypeScript support #106

merged 7 commits into from Apr 16, 2019

Conversation

yannickcr
Copy link
Contributor

@yannickcr yannickcr commented Apr 12, 2019

Linting of TypeScript files is currently handled by TSLint in our projects while JavaScript files are linted using ESLint.

The consequences are:

  • We're using different rules between JavaScript and TypeScript files.
  • When it's not the case, the linting configuration is duplicated between two tools.

Also the TSLint team has announced that TSLint will be deprecated this year in favor of ESLint: TSLint in 2019.

This change adds an algolia/typescript configuration that uses packages from TypeScript ESLint:

  • Replace babel-eslint by @typescript-eslint/parser so ESLint can parse TypeScript files.
  • Add TypeScript rules from @typescript-eslint/eslint-plugin. The configuration should be the same as the current TSLint one. Let me know if I missed some rules here.
  • Disable some ESLint rules that are superset by @typescript-eslint/eslint-plugin ones.

Additionally the Readme was updated with two new section "TypeScript" and "TypeScript with React"

@vvo
Copy link
Contributor

vvo commented Apr 12, 2019

Hey there, I trust you to upgrade this repository, if there's any breaking change (i.e. if I upgrade, without any change on my side since not using TS, will it still work the same?), do a major and add the necessary changes that needs to be implemented, thanks!

README.md Outdated Show resolved Hide resolved
base.js Outdated Show resolved Hide resolved
@iam4x
Copy link
Contributor

iam4x commented Apr 12, 2019

Hey 👋

I think it should be better to expose the configuration like the algolia/flowtype one. So we can opt-in for the one we want to use.

@yannickcr
Copy link
Contributor Author

yannickcr commented Apr 12, 2019

@vvo FMPOV in the current implementation it would totally be a breaking change for 2 reasons:

  • You need to install some new dependencies @typescript-eslint/parser, @typescript-eslint/eslint-plugin and typescript.
  • @typescript-eslint/parser support both JavaScript and TypeScript files, but does not support ECMAScript experimental features like babel-eslint, which can be problematic if you're using some in your codebase.

@iam4x Very good idea! I think it would also solve the problems described above (we could keep babel-eslint as default parser and only switch to @typescript-eslint/parser in TypeScript configuration).

The only detail is that you'll have to put the TypeScript configuration at the end of your extends so the parser is properly overridden:

Parser is set to babel-eslint by algolia/react configuration:

module.exports = {
  extends: ['algolia/typescript', 'algolia/react'],
};

Parser is set to @typescript-eslint/parser by algolia/typescript' configuration:

module.exports = {
  extends: ['algolia/react', 'algolia/typescript'],
};

But this detail could be described in the Readme.

@yannickcr
Copy link
Contributor Author

@iam4x I made the changes you suggested, is it what you had in mind ? (Usage example)
@vvo It should also fix the potential compatibility issues (no more changes to the existing configurations).

@iam4x
Copy link
Contributor

iam4x commented Apr 12, 2019

Yes exactly so we don't break anything for current users and still support TypeScript <3

Thanks!

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

LGTM (still as a major to be sure)

@francoischalifour
Copy link
Member

That's a great initiative!

Out of curiosity, did you try running this setup on one of our projects? (InstantSearch.js for instance). Would that involve some consequent changes in our linting usage? (formatting, ignoring rules in specific lines, etc.)

@yannickcr
Copy link
Contributor Author

@francoischalifour Yes, I tested it on InstantSearch.js, here's the result:

/scripts/jest/matchers/__tests__/toWarnDev-test.ts
  20:13  error  Unexpected console statement  no-console
  36:13  error  Unexpected console statement  no-console
  44:13  error  Unexpected console statement  no-console

/scripts/jest/matchers/toWarnDev.ts
   5:3   error  ES2015 module syntax is preferred over custom TypeScript modules and namespaces  @typescript-eslint/no-namespace
  26:32  error  Unexpected console statement                                                     no-console
  30:5   error  Unexpected console statement                                                     no-console
  37:5   error  Unexpected console statement                                                     no-console

/src/connectors/range/__tests__/connectRange-test.js
   81:11  error  Identifier 'facets_stats' is not in camel case  @typescript-eslint/camelcase
  189:11  error  Identifier 'facets_stats' is not in camel case  @typescript-eslint/camelcase

/src/helpers/__tests__/highlight-test.js
  16:3  error  Identifier 'price_range' is not in camel case    @typescript-eslint/camelcase
  19:3  error  Identifier 'free_shipping' is not in camel case  @typescript-eslint/camelcase

/src/helpers/__tests__/snippet-test.js
  16:3  error  Identifier 'price_range' is not in camel case    @typescript-eslint/camelcase
  19:3  error  Identifier 'free_shipping' is not in camel case  @typescript-eslint/camelcase

/src/lib/utils/getContainerNode.ts
  3:1  error  Missing JSDoc @returns for function                  valid-jsdoc
  3:1  error  Missing JSDoc for parameter 'selectorOrHTMLElement'  valid-jsdoc

/stories/query-rule-custom-data.stories.ts
  40:21  error  Arrow function expected a return value   array-callback-return
  43:19  error  Arrow function expected no return value  consistent-return

✖ 17 problems (17 errors, 0 warnings)
  • The comments to ignore some rules must be updated. Ex: tslint:disable:no-console => eslint-disable no-console, eslint-disable camelcase => eslint-disable @typescript-eslint/camelcase
  • A new @typescript-eslint/no-namespace error was detected (was ignored by TSLint)
  • Some lint errors in .ts and .tsx files since those files were not validated by ESLint before

Beside that, there should not have any changes in our linting usage.

Copy link
Member

@francoischalifour francoischalifour left a comment

Choose a reason for hiding this comment

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

That's super cool! Since I don't see anything breaking other configurations we can release this as a minor IMO.

@iam4x
Copy link
Contributor

iam4x commented Apr 15, 2019

Yes I think minor is good as well, for reference here are the rules of semver:

  • MAJOR version when you make incompatible API changes
  • MINOR version when you add functionality in a backwards-compatible manner
  • PATCH version when you make backwards-compatible bug fixes

Thanks guys, I will refactor my eslint config with this 🎉

'@typescript-eslint/ban-ts-ignore': ['off'],
'@typescript-eslint/camelcase': ['error'],
'@typescript-eslint/class-name-casing': ['error'],
'@typescript-eslint/explicit-function-return-type': ['off'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@yannickcr yannickcr Apr 15, 2019

Choose a reason for hiding this comment

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

No objection on this but it will add a little more work to type our code. There will be 7 errors to fix in InstantSearch.js but it should not be too difficult.

'@typescript-eslint/class-name-casing': ['error'],
'@typescript-eslint/explicit-function-return-type': ['off'],
'@typescript-eslint/explicit-member-accessibility': ['error'],
'@typescript-eslint/generic-type-naming': ['off'],
Copy link
Contributor

@samouss samouss Apr 15, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@yannickcr yannickcr Apr 15, 2019

Choose a reason for hiding this comment

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

No objection on this. There will be 10 errors to fix in InstantSearch.js, I'll need a little help to fix them but nothing insurmountable.

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.

None yet

6 participants