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

test: make common.noop a getter #12732

Closed
wants to merge 2 commits into from
Closed

Conversation

aqrln
Copy link
Contributor

@aqrln aqrln commented Apr 28, 2017

Make common.noop a getter that returns a new function object each time
the propery is read, not the same one. The old behavior could possibly
lead to subtle and hard to catch bugs in some cases.

Refs: #12711 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Make `common.noop` a getter that returns a new function object each time
the propery is read, not the same one. The old behavior could possibly
lead to subtle and hard to catch bugs in some cases.

Refs: nodejs#12711 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 28, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green. I'd be just as happy (maybe even slightly happier) to get rid of common.noop altogether, but this works for me!

@aqrln
Copy link
Contributor Author

aqrln commented Apr 28, 2017

@@ -24,7 +24,6 @@ const common = require('../common');
const assert = require('assert');
const events = require('events');


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, unintentional style change. I think I had written something there and then deleted too much.

@not-an-aardvark
Copy link
Contributor

not-an-aardvark commented Apr 28, 2017

I don't have a strong opinion on this change either way, but if common.noop returns a new function on each access, why is it better than just using () => {}? In cases where it does matter, it seems like it might be confusing for two accesses to common.noop to return different function instances. For example, if I was looking at this code and I didn't have additional context, I would assume that all the listeners were strictly equal to each other.

@Fishrock123
Copy link
Contributor

What if we Object.freeze() the function?

Object.defineProperty(exports, 'noop', {
enumerable: true,
get: () => () => {}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about keeping the old noop and add

export.newNoop = () => { return () => {}; };

@refack
Copy link
Contributor

refack commented Apr 28, 2017

+0.5
Although it's new syntax () => {} can be considered as an idiom meaning noop...

@benjamingr
Copy link
Member

I think at this point writing () => {} is shorter than writing common.noop and we should consider just using that instead.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 29, 2017

Even better, we should adjust our lint rules to allow unused arrow fn parameters to be _. e.g.

_ => {}

@refack
Copy link
Contributor

refack commented Apr 29, 2017

@Fishrock123 although _ => {} is shorter, IMHO () => {} is more idiomatic (e.g. MDN)

@jasnell
Copy link
Member

jasnell commented Apr 29, 2017

get rid of common.noop altogether

It was just added. A key reason I added this was to make it explicit that the function in question is intentionally a non-op. I'm definitely -1 on reverting back to the old approach. I'm also perfectly fine with cases that are better off not using common.noop simply not using it. In the original PR that added it I intentionally left one of the tests unchanged specifically for that reason.

I'm -1 on this PR.

@benjamingr
Copy link
Member

I'd just like to point out JavaScript already ships with a well known noop function - Function.prototype, that is guaranteed to take any number of arguments, return undefined and cause no side effects.

I still think a syntactical construct (like () => {}) or a built in (like Function.prototype) are clearer - but honestly all these ways are pretty obvious anyway and I don't think there is very much to gain/lose here.

@aqrln
Copy link
Contributor Author

aqrln commented Apr 29, 2017

@jasnell should we then maybe warn about the difference between common.noop and () => {} in test/README.md?

@not-an-aardvark
Copy link
Contributor

To me, it seems like if we have to document the gotchas of a noop function, it's already more trouble than it's worth.

@aqrln
Copy link
Contributor Author

aqrln commented May 4, 2017

I think at this point this PR is superseded by #12822, so I'm going to go ahead and close it to clean up the backlog of PRs a bit :) Reopening is cheap if there ever be a need to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants