Pioneer's linter issues #509
Conversation
|
I tried to test as much as I could, unfortunately most of the fixes are regarding |
|
Needs a merge from development to resolve conflict after #514 was merged. In that PR though, I did break the linter by using double quotes instead of single quotes: in I tested storage app didn't find any issues. In the content working group I'm observing a bug after creating an opportunity and trying to apply I get an empty loading page..
Which goes on forever, only after adjusting the windows size do I get:
I then had to switch to another app and come back to the working group app for application form to appear when applying..
Will test if this behavior also happens on current development branch. During this test I did see the following error in console which might explain what is going on. |
|
Resolved the conflicts with |
|
The issue described above is also found on development branch, so it was NOT introduced in this PR. |
mnaamani
left a comment
mnaamani
left a comment
There was a problem hiding this comment.
I can't find anything broken following some manual testing (using the apps)
|
There is one issue I uncovered doing some further tests now. Converting to draft and will add a fix soon. |
|
Doing some more extensive tests in Nesting jsx like that results in a string containing This should be fixed now. |
|
Perhaps we can merge Lezek123#6 to add the linter check to the CI checks Also I'm thinking of merging this work into the upcoming release branch. |
|
I did some tests and I noticed that there are a few different approaches we could use to add linting into the CI:
For more context see: https://github.com/Lezek123/substrate-runtime-joystream/commits/break-ci-lint-check |
I'm happy with that approach except I'm not sure if there is any added value in doing the linter check on more than one platform. There is no point in removing it now, unless we need to reduce the number of concurrent jobs. LGTM |
3 participants
This PR includes some adjustments to the Pioneer's eslint rules and fixes all the remaining eslint errors. This should allow us to add the linter check for Pioneer as part of the CI again and start paying attention to it from now on. Overall I think Pioneer's codebase is now going to be in a considerable better shape!
Some rules that I removed:
And the reasoning behind this
@typescript-eslint/camelcase- this one doesn't make much sense for Pioneer, because it would force us to use camelcase for all objects' property names etc., which as I experienced, can cause some issues withStructsthat we use to interact with the runtime (if we use camelcase and runtime uses snakecase, the properties may not get populated correctly ie. when sending extrinsics)react/prop-types- normally this rule checks if we havepropTypescorrectly defined in all of the react components, but it seems to give some unreliable results when working with TypeScript (ie. sometimes using a ts type for props is enough, but sometimes eslint has some trouble with infering the right types etc.). TypeSciprt would normally give us a build error if we use incorrect typing for props, so I'd say this rule isn't really necessary.new-cap- this rule gives us errors in all places we usenew lowercaseClassName(), but this usually regards classes imported from packages like@polkadot/types(ie.u16,u32etc.) and not the ones we define ourselfs (which give us other type of error).@typescript-eslint/interface-name-prefix- this rule I think is not very useful in general. According to it it's a bad practice to use interface names likeIConfig, but sometimes I think it just makes sense, ie. we have a class likeFinalizationData, which we use to register the type for the api and an additional interface/type for that class calledIFinalizationData(which we use forJoyStructfor example)Some common problems I fixed:
(> 10 errors per rule, each of those has a separate commit)
--fixflag. This handles some whitespace errors, swtiches some characters like',",;and,in places where those changes shouldn't really affect anything etc. (just making things consistent). Applying this fix already removed ~80% of the errors. Since I think it's very useful, I also made it available throughyarn workspace pioneer lint-autofix.no-mixed-operators(e4548e3) - this seems to be reasonable rule preventing the use ofA && B || Cwithout parenthesis (which may be confusing). Here I think that pretty much for all the occurences it was enought to just replace those with(A && B) || C(logically they already made sense)react/no-unescaped-entities(3d08c44) - this rule prevents the use of characters like',",<and>inside jsx direct-child strings, ie.<div>></div>, since there is a high chance that it is a mistake. In case of Pioneer those weren't mistakes, but I think the rule is still worth preserving so I fixed those by changing strings likeBackers' staketo analogous expressions, like:{'Backers\' stake'}.typescript-eslint/no-use-before-define(9f3cb13) - the name of the rule is pretty self-explainatory. Although we can usefunctionsetc. before we define them, it's better to avoid this as this is normally not the case withlet,constetc. The fix here was mostly about moving the code around, so that everything is correctly defined before used.eqeqeq(c6549d8) - this rule prevents us from using==and!=. Assuming our TS typing is correct, it was safe to just replace those with===and!==, so that's what I did here.@typescript-eslint/no-empty-function(0b64ddf) - according to this rule using syntax like() => {}may be confusing, because it's not clear whether we want the function to just do nothing or return and empty object. Changing it to() => {/* do nothing */}is enough to signal the intention and silence those kind of errors.react/display-name(cd2d440) - this I think is a very good rule, which forces use to set component display name in all places where it cannot be easily derived (ie. from class/function name), so mostly in our HOCs. This is also mentioned in the react documentation (https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging). Thanks to fixing this we will now get a cool, descriptive component name likewithMembershipRequired(SomeComponent)instead ofundefinedin react warnings/errors, which will greatly increase the readability of those. This fix required a bit more coding, but in that case I'm pretty sure it's going to be worth it.react/jsx-key(da495fb) - React requires to specifykeyprop when using lists/arrays of jsx elements. Not using those results also in a console warning/error, since it may negatively affect the speed of rendering the lists etc. Fixed by addingkeyprop where necessary.no-mixed-spaces-and-tabs/no-tabs(150a28b) - those were some whitespace issues that could reduce the readibility of the code, but for some reason they couldn't be fixed automatically.After fixing all the issues listed above, there were still 67 remaining errors. I fixed those in the last commit (2e67b25). Some I silenced using
eslint-disable-next-line, like when we were usingasync functionwithoutawaitin mock transports (since they needed to return the promise in order to be consistent with the "real" transport).Here's the list of issues fixed in that commit: