[NON-BREAKING] ES2015 Proxy approach (requires Node.js 6) by schnittstabil · Pull Request #32 · sindresorhus/pify

schnittstabil · 2016-09-17T18:43:12Z

See: #29

schnittstabil · 2016-09-17T18:48:47Z

argumentList can just be passed as an array.

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
            };
        }

sindresorhus · 2016-09-17T19:01:05Z

Not entirely sure what you're asking about in that example. Can you clarify?

sindresorhus · 2016-09-17T19:03:22Z

index.js

-				for (let i = 1; i < arguments.length; i++) {
-					results[i - 1] = arguments[i];
-				}
+				const [, ...results] = arguments;


No need to use arguments. Just use the rest operator in the function. Can switch to arrow function then too.

Sadly, arrow functions don't have arguments...

Re-read my comment. Rest parameters ftw ;)

@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]); } });

First. We should optimize for the common case.

schnittstabil · 2016-09-17T19:09:15Z

I don't see benefits, e.g every get returns a newly created function:

            return function() {
                return cached(arguments); // or array version of arguments
            };

sindresorhus · 2016-09-17T19:31:14Z

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.

schnittstabil · 2016-09-17T19:36:48Z

65d5fc9: Maybe it's not covered by the tests, but I think you have to change the return value of get too!?

sindresorhus · 2017-01-21T08:09:30Z

@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: const pify = require('pify/proxy'); (Or a better name?)

schnittstabil · 2017-01-21T11:44:10Z

require('pify/proxy') sounds reasonable.

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.

sindresorhus · 2017-01-21T11:58:44Z

Could you move it to a separate file?

schnittstabil · 2017-01-21T13:55:49Z

Sorry, not until Monday.

schnittstabil · 2017-01-23T19:43:47Z

@sindresorhus Doesn't really LGTM, but proxy stuff moved into proxy.js.

Remaining issues I see so far:

How should we deal with [[Set]]? For example:

const fsP = pify(fs);
fsP.stat = p => new Promise(…);
// and
fsP.stat = (p, cb) => {…};

How should we deal with non-writable, non-configurable properties? For example:
```
const fsP = pify(fs);
Object.freeze(fs);
fs.stat === fsP.stat // <- MUST be true!?
```
From [[Get]] (P, Receiver):
[[Get]] for proxy objects enforces the following invariants:
- The value reported for a property must be the same as the value of the corresponding target object property if the target object property is a non-writable, non-configurable own data property.

schnittstabil · 2017-01-23T19:47:31Z

test-proxy.js

+import fs from 'fs';
+import test from 'ava';
+import pinkiePromise from 'pinkie-promise';
+import fn from './proxy';


test-proxy.js is (almost) an one-to-one duplicate of test.js 😒

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.

thom4parisot · 2017-01-24T13:46:21Z

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 (master to latest, node6-proxy to node6-proxy or next)?

The breaking change would occur when you decide to move the node6-proxy to latest.

Just a thought, not even an opinion :-)

schnittstabil · 2017-01-24T15:29:59Z

@oncletom I'm not sure if I understand you correctly. As far as I can see, merging this PR into master is not a breaking change (anymore):

nothing changed for node4 and node6 users if they require('pify')
node6 users can also require('pify/proxy'), thus proxies are only an additional functionality

thom4parisot · 2017-01-24T15:36:33Z

@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 Proxy enabled.

sindresorhus · 2017-01-31T08:37:32Z

Just let them override. We shouldn't do anything. Pretty edge-casy anyways.
We document it as a caveat. I really doubt anyone will hit this. I've yet to see Object.freeze() actually used. And if they try using it, it will throw:

'get' on proxy: property 'stat' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value

sindresorhus · 2017-01-31T08:39:27Z

test-proxy.js

+import fs from 'fs';
+import test from 'ava';
+import pinkiePromise from 'pinkie-promise';
+import fn from './proxy';


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.

sindresorhus · 2017-01-31T08:41:25Z

proxy.js

+
+	for (let i = 0; i < arguments.length; i++) {
+		args[i] = arguments[i];
+	}


Could you also integrate 65d5fc9? Would like to remove this boilerplate code.

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.

Alright, let's just hold off on changing that until Node.js 8 which should fix the perf deoptimization.

sindresorhus · 2017-05-27T13:56:30Z

@schnittstabil I totally forgot about this one. Anything left before we can merge? Can you fix the merge conflict?

sindresorhus reviewed Sep 17, 2016

View reviewed changes

schnittstabil force-pushed the proxy branch from f4ff177 to 6c25373 Compare January 23, 2017 19:23

schnittstabil commented Jan 23, 2017

View reviewed changes

schnittstabil changed the title ~~[BREAKING] ES2015 Proxy approach requires Node.js 6~~ [NON-BREAKING] ES2015 Proxy approach (requires Node.js 6) Jan 24, 2017

sindresorhus requested changes Jan 31, 2017

View reviewed changes

sindresorhus mentioned this pull request Feb 8, 2018

Promisifying all methods of a ES6 Class instance doesn't seem to work #57

Closed

sindresorhus force-pushed the master branch from 4fcca32 to fb15a12 Compare September 29, 2018 18:51

sindresorhus mentioned this pull request Mar 16, 2019

Return ES2015 Proxy object #29

Closed

schnittstabil and others added 2 commits March 21, 2019 07:45

add ES2015 Proxy approach (requires Node.js 6)

7df08f9

Spread arguments

27cac16

SamVerschueren force-pushed the proxy branch from 6c25373 to 27cac16 Compare March 21, 2019 07:20

sindresorhus closed this May 29, 2019

frangio mentioned this pull request Jun 20, 2019

Implement using Proxy #73

Merged

sindresorhus deleted the proxy branch January 24, 2021 05:59

Uh oh!

Conversation

schnittstabil commented Sep 17, 2016

Uh oh!

schnittstabil commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sindresorhus commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schnittstabil Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schnittstabil commented Sep 17, 2016

Uh oh!

sindresorhus commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schnittstabil commented Sep 17, 2016

Uh oh!

sindresorhus commented Jan 21, 2017

Uh oh!

schnittstabil commented Jan 21, 2017

Uh oh!

sindresorhus commented Jan 21, 2017

Uh oh!

schnittstabil commented Jan 21, 2017

Uh oh!

schnittstabil commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thom4parisot commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schnittstabil commented Jan 24, 2017

Uh oh!

thom4parisot commented Jan 24, 2017

Uh oh!

sindresorhus commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schnittstabil Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented May 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

schnittstabil commented Sep 17, 2016 •

edited

Loading

sindresorhus commented Sep 17, 2016 •

edited

Loading

sindresorhus Sep 17, 2016 •

edited

Loading

schnittstabil Sep 17, 2016 •

edited

Loading

sindresorhus commented Sep 17, 2016 •

edited

Loading

thom4parisot commented Jan 24, 2017 •

edited

Loading

schnittstabil Jan 31, 2017 •

edited

Loading

sindresorhus May 27, 2017 •

edited

Loading