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
Conversation
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! |
Hey 👋 I think it should be better to expose the configuration like the |
@vvo FMPOV in the current implementation it would totally be a breaking change for 2 reasons:
@iam4x Very good idea! I think it would also solve the problems described above (we could keep The only detail is that you'll have to put the TypeScript configuration at the end of your Parser is set to module.exports = {
extends: ['algolia/typescript', 'algolia/react'],
}; Parser is set to module.exports = {
extends: ['algolia/react', 'algolia/typescript'],
}; But this detail could be described in the Readme. |
@iam4x I made the changes you suggested, is it what you had in mind ? (Usage example) |
Yes exactly so we don't break anything for current users and still support TypeScript <3 Thanks! |
There was a problem hiding this 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)
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.) |
@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)
Beside that, there should not have any changes in our linting usage. |
There was a problem hiding this 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.
Yes I think minor is good as well, for reference here are the rules of semver:
Thanks guys, I will refactor my eslint config with this 🎉 |
rules/typescript.js
Outdated
'@typescript-eslint/ban-ts-ignore': ['off'], | ||
'@typescript-eslint/camelcase': ['error'], | ||
'@typescript-eslint/class-name-casing': ['error'], | ||
'@typescript-eslint/explicit-function-return-type': ['off'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on our usage but it might be interesting to enable it with both option enable.
There was a problem hiding this comment.
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.
rules/typescript.js
Outdated
'@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'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to enable it to force the usage of TXxxx
inside a generic.
There was a problem hiding this comment.
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.
Linting of TypeScript files is currently handled by TSLint in our projects while JavaScript files are linted using ESLint.
The consequences are:
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:babel-eslint
by@typescript-eslint/parser
so ESLint can parse TypeScript files.@typescript-eslint/eslint-plugin
. The configuration should be the same as the current TSLint one. Let me know if I missed some rules here.@typescript-eslint/eslint-plugin
ones.Additionally the Readme was updated with two new section "TypeScript" and "TypeScript with React"