Opened 20 months ago

Last modified 19 months ago

#6445 new change

[meta] Make use of default function parameters

Reported by: mjethani Assignee:
Priority: P4 Milestone:
Module: Unknown Keywords:
Cc: agiammarchi, sergz, sebastian, hfiguiere, fhd, greiner, kzar Blocked By: #6505
Blocking: #6425 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

ES6 supports default function parameters.

We have code like this, for example:

function makeSelector(node, selector)
{
  let index = positionInParent(node);

  if (index > 0)
  {
    let newSelector = `${node.tagName}:nth-child(${index})`;
    if (selector)
      newSelector += " > " + selector;

    return makeSelector(node.parentNode, newSelector);
  }

  return selector;
}

Unfortunately everywhere this function is called we are passing an empty string to it as the second argument. The second argument is in fact an implementation detail, the caller shouldn't have to pass a default value here. In such cases the function should already have default values assigned.

What to change

Find instances in the code where it would benefit from the use of default function parameters and make the appropriate changes.

Change History (9)

comment:1 Changed 20 months ago by mjethani

  • Blocking 6425 added

comment:2 Changed 20 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 follow-up: Changed 19 months ago by kzar

  • Priority changed from Unknown to P4
  • Ready set

Seems like default function arguments are supported by all our supported browsers and I don't see a down side of using them. Unless someone disagrees or I missed something I think we can go ahead with this one.

Please could you open an issue to update our coding style rules page about this? We should also open separate issues if we need to make changes to our other repositories.

comment:4 in reply to: ↑ description Changed 19 months ago by sebastian

I don't have any objections either, so far. But I generally (not only for this issue), prefer to discuss new suggested coding practices based on a patch showing the changes to our code base.

Replying to mjethani:

Unfortunately everywhere this function is called we are passing an empty string to it as the second argument. The second argument is in fact an implementation detail, the caller shouldn't have to pass a default value here. In such cases the function should already have default values assigned.

For completeness, there are also examples, where the other way around our existing code already emulates default parameters, though boilerplate that is obsolete now, e.g. follwoing example from lib/whitelisting.js in adblockpluschrome:

exports.checkWhitelisted = (page, frame, typeMask) =>
{
  if (typeof typeMask == "undefined")
    typeMask = RegExpFilter.typeMap.DOCUMENT;

  ...
};

If we start using default function parameters, code like this should be changed to:

exports.checkWhitelisted = (page, frame, typeMask = RegExpFilter.typeMap.DOCUMENT) =>
{
  ...
};

comment:5 in reply to: ↑ 3 Changed 19 months ago by agiammarchi

Replying to kzar:

Seems like default function arguments are supported by all our supported browsers and I don't see a down side of using them.

That very same MDN page states Opera 45 is the one supporting them. I wouldn't be surprised if that'd be just another typo in their JSON but when in doubt, since I'm updating and adding real tests as we go/update code, feel free to verify live browsers features.

comment:6 Changed 19 months ago by agiammarchi

FYI I've double checked myself and Opera 36 indeed has no issues with default parameters. It looks like there could be some issue with non defaults after defaults (non tested behavior) but I'll write a test myself to be sure it works.

MDN bug filed

edit tested also regular parameters after default parameters and everything seems to be just fine

Last edited 19 months ago by agiammarchi (previous) (diff)

comment:7 Changed 19 months ago by mjethani

  • Component changed from Platform to Unknown
  • Review URL(s) modified (diff)
  • Summary changed from Make use of default function parameters to [meta] Make use of default function parameters

comment:8 Changed 19 months ago by mjethani

I've made this a meta issue, I'll file separate ones for Core and Platform. Meanwhile let's keep any discussions about this topic here.

comment:9 Changed 19 months ago by mjethani

  • Blocked By 6505 added
Note: See TracTickets for help on using tickets.