[minor] Relax the no-param-reassign warning

[monfera]

Currently, a stringent version is active. Ie. it doesn't just complain on parameter reassignment. It also complains if you want to set some inner property of what gets passed as a parameter. Which is the intent, and often used, eg. when building tree or generally recursive structures, indices etc. Example

The no-param-reassign is a clear misnomer in this case. The word parameter has precise meaning. In this case, it's the binding of the argument object to the name mapNode. The parameter is mapNode. That line does not reassign the parameter.

Would be great to deactivate it for the property update case and leave it in place for the reassignment of the binding eg. const foo = x => { x++ } though even such use isn't a huge deal in a library (popular libraries sometimes have if(!Array.isArray(arg2)) arg2 = [arg2] or some such)

It's shades of gray and subjective: my feel is that leaning on the stricter side with warnings and errors is great practice for web apps but feels less useful for library code. Read support for this kind of lib vs app distinction, maybe I'll run into it again. Eg. is the above referred code line in group_by_rollup problematic in data processing code that warrants a noise line? I'm OK if we are explicit about our goals and state that we're inching toward immutability; in this case we could chat about the goal, then put the rule in to help move toward the goal. The linter rule alone won't help much if this is the goal. Also, immutability has a price.

It also feels off to see a linter warning and not do something about it. It's nice to see the gutter with no discolorations, so I'll prefix that line with // eslint-disable-next-line no-param-reassign which is better than leaving in a warning (even if I currently disagree with the necessity for the warning for the stricter interpretation).

Some of the most popular libraries that do nontrivial things don't enforce a large fraction of the rules just because they are around. Often for some good reason, and they don't typically excuse them with noisy // eslint-disable-next-line no-param-reassign lines. Would be interested in how y'all see the webapp vs library use case, and why there should, or should not be a distinction.

[monfera]

While it's definitely possible to work around the sometimes compounding linter warnings, which involve various pros and cons, stuff like this could be looked at together:

Here, there is a pattern of cleanly nesting Canvas2d blocks without the dreaded, otherwise easy contamination of the Canvas2d state: withContext saves and restores the canvas state. So the motive is, decent modularity, almost like components, with the otherwise stateful Canvas2d.

withContext takes the context (ctx) parameter, and due to nesting, this identifier reoccurred, which is fine - you click on an occurrence and it'll bring you to the appropriate parameter list (though the indentation helps too). Moreover, there's not a lot of specific value in distinguishing - it's going to be the same context.

So we either need to rethink and maybe change this small utility so its usage is easy to make compliant, or start using ctx2, ctx3 etc. as we nest. Which is awkward, as it adds no info.

OK let's still do it. Next problem: the no-param-reassign issue 🤣 When trying to set them darn canvas context properties 🤣

Maybe what we could do:

• when adding a new rule, only merge if all new warnings got resolved
• the pros and cons of a rule could be individually valued
• the way of resolving could be discussed, because, while everything can be worked around, every advance in complying with newly imposed coding standards constraints might need changes that ignore some preexisting value in the code

Having said this, I can't rule out that there's a nicer solution than withContext, which can be found at some expense (time),—ideas welcome btw.—given the added lint constraints; so all I'm suggesting is that we resolve the warnings first, or at least have a quick chat per type of newly introduced warning, rather than causing warnings, as it admits technical debt even in the absence of doing so.

For withContext, maybe we can have something like a single global, outer withContext which captures the ctx; then, in the scenegraph render code, it can be called as

withContext(() => {

instead of

withContext(ctx, (ctx2) => {

However I'd hate to do

    // eslint-disable-next-line no-param-reassign
    ctx.lineJoin = 'round';
    // eslint-disable-next-line no-param-reassign
    ctx.strokeStyle = sectorLineStroke;
    // eslint-disable-next-line no-param-reassign
    ctx.lineWidth = sectorLineWidth;

and even this is a weird workaround:

    const myCtx = ctx;
    myCtx.lineJoin = 'round';
    myCtx.strokeStyle = sectorLineStroke;
    myCtx.lineWidth = sectorLineWidth;

When seeing this, I feel like I'm missing some obvious solution, or if we start pondering how to placate some rule in otherwise legit code, maybe it's a sign the rule has a bit of an overreach. Sure it's just a warning but it's not nice to keep the same set of warnings in perpetuity, we should solve them and make new ones 😄

[monfera]

Unrelated to the title, seemingly also overbearing rule: it looks impossible to have a comment row that follows the copyright notice on the top of the file. Example:

Unless there's a reason for prohibiting a comment line that happens to be on the top of the file (naturally, after the copyright notice) maybe this rule could also be removed or discussed