Skip to content
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

enforce function statements #381

Closed
davidjamesstone opened this issue Jan 11, 2016 · 15 comments

Comments

@davidjamesstone
Copy link

commented Jan 11, 2016

I was wondering about folks preference in regards to expression vs statements.

Clearly it's important to understand the differences between the two in regards hoisting and also the use cases of each but there are often times where either could be used.

I can't find a recommendation - is there a guideline or for this in standardjs?

I see there's the "Enforce Function Style (func-style)" rule in ES Lint. I don't think I'm suggesting this be implemented but it would be nice to have an official line on the preferred approach.

Many thanks

Dave

@dcousens

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

Could you post some example code? Probably easier to self-parse and understand what you're referring to?

@dcousens dcousens added the question label Jan 12, 2016

@davidjamesstone

This comment has been minimized.

Copy link
Author

commented Jan 12, 2016

Hi Daniel,

Thanks for your reply.

I'm generally referring to 'private' helper type functions that don't have value outside of their enclosing function. Take the save function below.

Here a function expression or declaration would suffice. For consistency, which one should I opt for?

/*
 * Example adding a comment with an optional replyToId
 */
function addComment (body, inReplyToId, callback) {
  // Define the save function with a function declaration
  function save (comment) {
    comment.save(function (err) {
      if (err) {
        callback(err)
      }
      callback(null, comment)
    })
  }

  // The above function could have been a function expression...
  // var save = function (comment) {
  //   comment.save(function (err) {
  //     if (err) {
  //       callback(err)
  //     }
  //     callback(null, comment)
  //   })
  // }

  if (inReplyToId) {
    Comment.findById(inReplyTo, function (err, doc) {
      if (err) {
        callback(err)
      }
      save(new Comment({ body: body, inReplyTo: doc.id }))
    })
  } else {
    save(new Comment({ body: body }))
  }
}

Hope this helps

Dave

@dcousens

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

I personally opt for function save, but I don't think there is a hard-rule for this in standard.

Maybe there should be.

@feross ?

@rstacruz

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

xo has one. I don't mind it.

@davidjamesstone

This comment has been minimized.

Copy link
Author

commented Jan 12, 2016

Likewise I have opted for function save () {...} in the past. This is the also the advice from Crockford.

More recently though I have noticed people using var save = function () {...} a lot more.

While I don't have a strong preference, I would like to see some consistency within code base/across teams.

@dcousens

This comment has been minimized.

Copy link
Member

commented Jan 12, 2016

More recently though I have noticed people using var save = function () {...} a lot more.

Probably because you have to use that syntax if you are using context capturing lambdas:

var save = () => {}
@cesarandreu

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2016

I use arrow functions with callbacks or when I need to preserve context.

I'm not sure this holds true anymore, since it seems most engines have improved this by now... But it used to be that the following would give you worse stack traces:

// Anonymous function
const foo = function () {}

// Could be fixed by giving it a name
const foo = function foo () {}

With babel the default behavior seems to be to name the function:

// Before
const foo = () => {}


// After
const foo = function foo () {}

I'm a fan of function declarations since it tends to encourages writing pure functions.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

Function declarations are preferred.

Function declarations have more intuitive hoisting behavior, i.e. you can use the function anywhere that's in-scope. With function expressions, only the variable is hoisted, so you can only use the function after the line where the assignment occurs.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

Just tried enabling func-style to enforce function declarations, while making an exception for arrow function expressions, and got this result:

# tests 427
# pass  331
# fail  96

Full output: https://gist.github.com/feross/0d57fdddde82f55d618a

It's a high number, so leaning toward not enabling this. Much as I like it. Maybe if we do standard-strict?

@feross feross added feature request and removed question labels Feb 4, 2016

@feross feross changed the title function expression vs statement enforce function statements Feb 4, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

For documentation, here's the rule I tested with:

"func-style": [2, "declaration", { "allowArrowFunctions": true }]

@feross feross added enhancement and removed feature request labels Feb 4, 2016

@feross feross referenced this issue Feb 4, 2016
@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

@feross how about V6? I think it is a reasonable enough change and absolutely in the spirit of our existing rules.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

@dcousens Agreed, but it's quite a bit of breakage (22% of the ecosystem).

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

@feross it is also a break that could probably be resolved with a regex. We could provide / run it for them even.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

For VIM users

%sno/var \(\.\+\) = function \.\*(/function \1 (/gc
@feross

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

closing - we can move discussion of potential "next gen" or big breaking rule to the standard-strict issue: #382

@feross feross closed this Feb 5, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants
You can’t perform that action at this time.