-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
New Typescript linting overriding user config and causing build failure #6871
Comments
I had the same issue but I found a solution: https://graphql-code-generator.com/docs/plugins/add In your Example:
|
I'm having the same issue: tslint.json rules seem to be ignored now in v3 and overwritten by the new defaults. Please send help. |
We've also upgraded to the latest react-scripts (3.0.0) in order to get rid of high vulnerabilities, but it broke our tslint configuration. No |
Our ESLint configuration is not currently configurable (as has always been the case) and is documented here: https://facebook.github.io/create-react-app/docs/setting-up-your-editor#displaying-lint-output-in-the-editor. Any changes you make to your configuration file will only affect your editor integration but not the Dev server or prod builds. We haven't ever supported TSLint so I'm not sure how the other comments are related. |
Ok, but why can't I configure my own rules? To comply with your rules, I would have to rewrite my code. Stupid breaking change... I also came back to revert to 2. |
@ianschmitz I think the confusion was that Typescript folks where using TSLint since the react-scripts had no TS linting support. We have entire projects built using custom rules, but now since TS linting is supported in v3, we are all stuck with pages of warnings and/or broken builds. While the JS community had somewhat the ease of greenfield development using react-scripts' blessed linting rules, the TS community is now coming onboard with custom opinionated rulesets for large existing projects. Proposals:
On a community aside, I didn't realize the react-scripts didn't allow JS projects to customize the linting rules. As someone that disagrees with the defaults of most linters (as well as my team), it feels very heavy-handed to including linting by-default without a way to customize. (obviously there's ejection, but it's far from an easy path to own) |
I purposely use eslint as the Typescript team stated that that is their preferred solution and are putting effort into eslint support, though I'm sure some folks here use tslint. But back to the comment by @ianschmitz . I understand that the linting config, which now supports Typescript, is it's own thing and doesn't use the lint config a developer may add but my issue is that this recent change broken my code, and it seems others too. I know moving to 3 means possible breaking changes but this wasn't in the release notes, probably because no one realised. The workaround @wilcoschoneveld mentioned will get me out of this for now but as @jadbox says, linting is opinionated and projects have rules in place and probably went through their own pain getting them agreed. I too think that having CRA linting errors stop a build is too heavy handed. I worry that option 2 would be too difficult to do and maybe the quickest short term solution would be to add a config option to allow a build to complete even when there are lint errors and maybe another to allow linting to be disabled so the user can use their own. |
@ianschmitz is this just a matter of looking to see if a user has provided an |
@ztolley The baked in linting configuration is how we've always provided CRA, including adding new rules between major versions which is similar to what is being mentioned in this thread. This thread is slightly different in that some (including myself) were using TSLint prior to 3.0. To clarify, we did mention that the added TypeScript linting was in the 3.0 release notes and listed as a breaking change. I will discuss the possibility of extending the ESLint config with the team which i think would satisfy what most of you are looking for (i know i would use it personally), but i can't promise anything as it's a fairly significant change to how we've been providing CRA for a few years now. |
This is very frustrating. |
@dmitryxcom Just to be clear, by "the fix", are you referring to 3.0, or to some subsequent PR? |
TSLint is getting deprecated, as its own homepage says: I'm not sure what longer term strategy you're suggesting if even TypeScript itself is officially recommending a move to ESLint (which we have done). Create React App 3.0 aligns the linting rule with the same rules that have always been enabled in the JavaScript version. If there are specific rules that are causing you trouble, I'd appreciate a more detailed writeup about specifics. There is no need to make this issue emotional. We're all people and are trying to do what's good for the project and most users. I'm sorry upgrading created issues for your existing setups — but eventually you'd have to make those changes anyway, as TSLint will be deprecated by its maintainers. |
I'd like to emphasize Create React App ships with a very relaxed rule set that only includes the bare minimum of things that protect against real bugs. We don't ship any stylistic rules or rules that are largely a matter of opinion. (The only exception is the React Hooks rules which we intend to enforce more strongly.)
Can you give me an example of which rules you're bumping into? |
and similar. While the deprecation is on the roadmap the current state of eslint does not allow it to be a drop-in solution since it misses the most important type-based TS checks tslint provides (and for the majority of other checks there's prettier). Making it the default in CRA is a bit too early a call, in my opinion, and that is what is frustrating about the decision to make it default in CRA3, which goes beyond being opinionated, and becomes pushy and limiting. The deprecation notice clearly warns about eslint not being ready as a replacement:
My strategy is to wait until esint is fully compatible, which I argue should be CRA strategy as well, or at least, CRA should provide an opt-out of ts eslint linting until that point is reached. Like you point out, the migration will happen eventually, but the time is not here yet.
I've opted out of CRA3 for now with a todo to file specific bugs if I find the time to do that work. |
I think it would be very reasonable to expect at least an opt-out until palantir/tslint#4534 and typescript-eslint/typescript-eslint#36 are closed |
Do I understand correctly that with #6915 there's currently no way to configure eslint? In which case, it can not be considered a tslint replacement at this point? |
@ianschmitz Thanks for your reply and your patience. In my case I can get around it for now. I feel for the people who can't and I think an opt out would help them and be a quick win. I also agree that a measured approach to addressing this and providing further flexibility is the right thing to do, rather than rush into a solution that makes things worse with regards to picking up a users config. |
I would imagine more than a handful of organisations have CRA-based projects with CRA 3 breaks any such project by enforcing a different set of rules and it's a shame to eject because of it. While moving towards ESLint and providing some reasonable defaults for linting are both very reasonable goals, I think the existence of local tslint or eslint configuration files should definitely override this. It's likely most developers aren't even aware of TSLint's future deprecation plans yet, so this is likely a very unpleasant surprise while updating projects dependencies. Perhaps a more reasonable migration path would be to display a deprecation warning about TSLint configuration, so users can start migrating their rules. |
You're welcome to continue using TSLint alongside CRA if you wish. I do so myself for a few promise rules that aren't in ESLint yet. How does CRA break your project? What rules are causing issues for you? |
@ianschmitz in my case, the linting is not necessarily breaking, but confusing in a few ways:
@gaearon rules I'm bumping into:
titles.sort((titleA, titleB) => {
switch (titleSortBy) {
case 'name': {
return getAlphabeticalSortValue(titleA.name, titleB.name);
}
case 'type': {
return getAlphabeticalSortValue(titleA.type, titleB.type)
}
case 'launchDate': {
return getNumericSortValue(titleA.launchDate, titleB.launchDate);
}
}
}); It looks like @ianschmitz already addressed this, though I'm not sure that PR also covers
// only run this effect when props.movieId changes
useEffect(() => {
props.dispatch(fetchTitle(props.movieId));
}, [props.movieId]); The example here makes sense since Even if I adopted all the rules above, there are additional rules I like that aren't in the CRA configuration, such as @ianschmitz I understand there may be a compelling philosophical reason to not allow linting customization in CRA, but is there a compelling code complexity reason? It seems like loading a user's custom |
facing a similar issue, excluded node_modules but it is still linting src folder in one of the npm packages. |
There also are code breaking rules in the current configuration! class Test {
constructor(a: number, b: string);
constructor(b: string);
constructor(aOrB: string | number, b: string) {
if (typeof aOrB === 'number') ...
}
} This code will break with this error:
This issue was reported here typescript-eslint/typescript-eslint#420 and the only correct solution is to disable EDIT:
|
Taking us way off topic here (sorry everyone :), buuuuut...
That only occurs if you strictly type the function return and/or use This can then be handled by instead using the following helper, which will trigger a type error if someone forgets to add a case to the switch because TS won't do an implicit never cast: function assertIsNever(value: never) {
return new Error(`Unexpected value ${JSON.stringify(value)}`);
}
/////
default:
// Argument of type '"b"' is not assignable to parameter of type 'never'.
throw assertIsNever(value); |
@dannycochran There isn't currently a PR up for extending the ESLint config yet, but internally we are looking into it. @bradzacher ESLint's create-react-app/packages/eslint-config-react-app/index.js Lines 82 to 89 in 57ef103
The linting rule provides a nicer DX as the TypeScript |
@ianschmitz - sorry, I meant that our rule ( What I was meaning is that our no-unused-vars is great for development because it can be non-build-blocking, but it's not great as a production build blocker because of the false positives. The best experience is to use our no-unused-vars in dev only, and |
We have started a PR to extend ESLint config: #7036. @bradzacher I see what you're saying now. Thanks for clarifying. I'm not sure what the best path forwards is. Perhaps with #7036, users could disable that rule if it's causing issues for them. They could enable |
As a community it is important to be respectful to one another. As for the issue itself, I'm currently using version 2.1.5. I can't continue using this version since |
Is there any way to completely disable tslint? |
I tried deleting
|
I am getting an error
|
@zwhitchcox https://github.com/arackaf/customize-cra#disableeslint is probably the best option right now. |
How can I run typescript-eslint on pre-commit? Let's say using lint-staged. I found the CRA linters config at package: |
@mbaranovski I've just executed |
@FredyC I dont have any eslint rules defined in my project, I'm only using CRA 3.0.0 defaults. When running |
Thanks @FredyC it works now :) |
Hm, I take it back, I thought that Running I had to run the ugliness of This should be definitely documented. It's great we get lint checks with |
I'm using simply a glob pattern, ./src/**/*.{ts,tsx} |
Having the ability to define/override rules in eslintrc.json is pretty important... |
I like to structure my TS code using namespaces to group functions, for example: export type Option<A> = Some<A> | None<A>
export namespace Option {
export const none = <A>(): Option<A> => new None<A>()
export const some = <A>(a: A): Option<A> => new Some<A>(a)
}
... CRA seems to want to tell me how to write my code:
This is a purely stylistic point - giving a compilation error! - and actually one that makes zero sense, since namespaces are actually useful. How do I get rid of this useless error? (the workaround, for those others frustrated with this limitation, is to declare a |
AFAIK namespaces are still not supported. Edit: Ah, so there is some experimental support ... https://babeljs.io/docs/en/babel-plugin-transform-typescript/ |
This one proves the rule that software updates are the scams to sell further fixes for those updates. |
Hi. I'm just chiming in here because I got side-swiped by having to upgrade create-react-app from version 2.1 to 3.0 this weekend, for a project that is fairly mature, in order to fix my build times for my react app. Please correct me if I'm wrong, but my understanding is:
If this is is the case, I am beyond disappointed in what the perceived user experience is for this. A developer is always going to want to toggle these settings. All of these settings may or may not directly apply to specific projects. And now the product Folks, this is terrible, and I would like to articulate that this lack of support is a big disappointment that makes me very keen to move away from |
If you want to totally disable the build warning overlays, you can add this css to your project as a short term / quick fix:
|
3.1 comes with support for custom eslint configuration, should that solve most of the problems here? |
@bugzpodder Over an hour later and I haven't had any luck getting that to work. Am I missing something? .env
package.json (The relevant parts) {
"dependencies": {
"react-scripts": "3.1.1"
},
"eslintConfig": {
"extends": [
"react-app",
"eslint-config-react-app"
],
"rules": {
"no-useless-escape": "off"
},
"overrides": [
{
"files": [
"**/*.ts?(x)"
],
"rules": {
"no-useless-escape": "off"
}
}
]
}
} |
@StJohn3D I would double check that there aren't any errors being swallowed -- #7530. Might wanna check by adding this line in to your |
@ztolley have you tried enabling extending our built-in ESLint config as described here: https://create-react-app.dev/docs/setting-up-your-editor/#experimental-extending-the-eslint-config? Let us know so I can close this if it solves your issues. |
The introduction of #6513 stops my application from compiling as it uses it's own Typescript linting rules instead of the rules I have in .eslintrc.js
Previous to CRA adding Typescript linting I created my own typescript lint config for eslint using
@typescript-eslint/eslint-plugin
and@typescript-eslint/parser
along with airbnb and prettier rules.Within the project there is typescript file generated by
graphql-code-generator
that breaks various linting rules, so has been added to a.eslintignore
file. When I run eslint the project passes with no errors.However, when I build the project or run it in dev mode 2 lint errors are raised, relating to the fact that import statements appear half way through the file. Using the .eslintignore file doesn't fix this problem, nor does updating my .eslintrc.js file to exclude the rule.
As a result I cannot build my project with CRA 3 and have had to revert to 2.
The text was updated successfully, but these errors were encountered: