-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
[NON-BREAKING] ES2015 Proxy approach (requires Node.js 6) #32
Conversation
If I understand you correctly, that would also imply: const processFn = (fn, opts) => function (args) {
// …
}
// …
get: (target, key) => {
// …
return function() {
return cached(arguments); // or array version of arguments
};
} |
Not entirely sure what you're asking about in that example. Can you clarify? |
index.js
Outdated
for (let i = 1; i < arguments.length; i++) { | ||
results[i - 1] = arguments[i]; | ||
} | ||
const [, ...results] = arguments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use arguments
. Just use the rest
operator in the function. Can switch to arrow function then too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, arrow functions don't have arguments
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-read my comment. Rest parameters ftw ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sindresorhus Which version do you prefer:
args.push((err, result, ...results) => {
if (err) {
reject(err);
} else if (opts.multiArgs) {
resolve([result, ...results]);
} else {
resolve(result);
}
});
or
args.push((err, ...results) => {
if (err) {
reject(err);
} else if (opts.multiArgs) {
resolve(results);
} else {
resolve(results[0]);
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First. We should optimize for the common case.
I don't see benefits, e.g every return function() {
return cached(arguments); // or array version of arguments
}; |
Easier to explain with code. This is what I was thinking: 65d5fc9 It's probably not entirely correct as some tests were failing, but I'm too sleepy to figure out why. But I hope it shows what I'm looking for. |
65d5fc9: Maybe it's not covered by the tests, but I think you have to change the return value of |
@schnittstabil I wanna ship this, but can't realistically just target Node.js 6 for a good while. How about we put this in a separate file and ship it now? People targeting Node.js 6 can use it, and it wouldn't make a difference for Node.js 4 users. For example: |
Honestly, I do not believe it works to all extend, but it seems ready for most use cases. Hence, I would suggest to either release it for internal usage only or document it as beta or similar. |
Could you move it to a separate file? |
Sorry, not until Monday. |
@sindresorhus Doesn't really LGTM, but proxy stuff moved into Remaining issues I see so far:
|
import fs from 'fs'; | ||
import test from 'ava'; | ||
import pinkiePromise from 'pinkie-promise'; | ||
import fn from './proxy'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test-proxy.js
is (almost) an one-to-one duplicate of test.js
😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok for now. We'll either replace proxy with the current approach or adopt both in the same API once we can target Node.js 6.
Don't you want to have two branches (one for non-proxy code, and one for node6-proxy), and to publish to two different npm tags ( The breaking change would occur when you decide to move the Just a thought, not even an opinion :-) |
@oncletom I'm not sure if I understand you correctly. As far as I can see, merging this PR into
|
@schnittstabil yeah sorry, I was not clear at all. I rephrased some of my thoughts and I totally get the fact you ship both approaches in the same branch, but with an additional entrypoint for people having a node environment which has |
|
import fs from 'fs'; | ||
import test from 'ava'; | ||
import pinkiePromise from 'pinkie-promise'; | ||
import fn from './proxy'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's ok for now. We'll either replace proxy with the current approach or adopt both in the same API once we can target Node.js 6.
proxy.js
Outdated
|
||
for (let i = 0; i < arguments.length; i++) { | ||
args[i] = arguments[i]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also integrate 65d5fc9? Would like to remove this boilerplate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the version below breaks optimization:
const processFn = (fn, opts) => function (...args) { …
And the following version don't work well too:
const processFn = (fn, opts) => function (args) { …
handler.get
have to be able to return processFn(x, opts)
with the function signature of x
. We could fix the signature in handler.get
, but that wouldn't be very sensible in my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's just hold off on changing that until Node.js 8 which should fix the perf deoptimization.
@schnittstabil I totally forgot about this one. Anything left before we can merge? Can you fix the merge conflict? |
4fcca32
to
fb15a12
Compare
See: #29