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

Explore using ES2015 Proxies #40

Open
gaearon opened this issue Feb 5, 2016 · 22 comments
Open

Explore using ES2015 Proxies #40

gaearon opened this issue Feb 5, 2016 · 22 comments

Comments

@gaearon
Copy link
Owner

gaearon commented Feb 5, 2016

They might make a ton of things easier.
We can fall back to the old method if they are unavailable.

I don’t have time for this right now but it’s a perfect opportunity to get involved with the project.
The tests should pass :D

If you decide to play with it, please post in this thread so we don’t duplicate the effort.

@gaearon
Copy link
Owner Author

gaearon commented Feb 5, 2016

Of the whole test suite, this test is the best place to get started. Disable all other tests, delete all the source code, and figure out how to make it pass with Proxies :-)

@ghost
Copy link

ghost commented Feb 5, 2016

:D I'm giving this a go now, but that shouldn't stop anyone else from taking a stab at it at the same time.

@akorchev
Copy link
Collaborator

akorchev commented Feb 5, 2016

On a related note this code fails in environments that support ES2015 classes natively. Will proxies solve this problem?

@ghost
Copy link

ghost commented Feb 6, 2016

Unless I'm missing something, the entire test suite has to be converted to run in the browser first, no?
As far as I can tell, FF is the only general release browser to support Proxies at the moment.

@gaearon
Copy link
Owner Author

gaearon commented Feb 6, 2016

On a related note this code fails in environments that support ES2015 classes natively. Will proxies solve this problem?

I don’t know, will they? If this code fails we should fix it.. I guess? I’m not sure this problem is related to Proxies though.

@gaearon
Copy link
Owner Author

gaearon commented Feb 6, 2016

Unless I'm missing something, the entire test suite has to be converted to run in the browser first, no?

Why would we run it in the browser? I want the tests to be executed in Node, like they are now. Is there no Node version with Proxies at all? That would be a bummer, and in this case, yeah, we’d have to use a browser runner like Karma.

@ghost
Copy link

ghost commented Feb 6, 2016

> process.version
'v5.5.0'
> var target = {}
undefined
> var p = new Proxy(target, {})
ReferenceError: Proxy is not defined
    at repl:1:13
    at REPLServer.defaultEval (repl.js:252:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:417:12)
    at emitOne (events.js:95:20)
    at REPLServer.emit (events.js:182:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
    at REPLServer.Interface._ttyWrite (readline.js:827:14)
> 

The latest node version doesn't look like it does.

@gaearon
Copy link
Owner Author

gaearon commented Feb 6, 2016

@dralletje
Copy link

According to node --v8-options | grep "in progress" the flag changed to --harmony_proxies :)

@ghost
Copy link

ghost commented Feb 6, 2016

Didn't get very far before having to move on to something else; might have time in a week or so, but hopefully someone beats me to it.

@ghost
Copy link

ghost commented Feb 6, 2016

https://github.com/danmartinez101/react-proxy
Converted over to use the browser for now(will figure out the node vs browser business after figuring out the actual implementation)

@gaearon
Copy link
Owner Author

gaearon commented Mar 5, 2016

Would you like to send a PR just for posterity’s sake?

@ghost
Copy link

ghost commented Mar 8, 2016

A PR of the browser tests or something else?

@gaearon
Copy link
Owner Author

gaearon commented Mar 8, 2016

Sorry, I thought you meant you had some prototype working. I’m not sure browser tests alone would be very useful. Did --harmony_proxies now work by the way?

@ghost
Copy link

ghost commented Mar 9, 2016

Ah, yea, I had something working but I didn't save it in a memorable place because it was only partially working. I'll see if I still have it around somewhere, but my hopes are not high. :/

I didn't try too hard at --harmony_proxies because the interface was different than all the documentation I was finding and my brain is to small for all that at once.

@tanem
Copy link
Contributor

tanem commented Mar 12, 2016

The latest node version doesn't look like it does.

From what I can see, latest Node stable is using a version of V8 that doesn't support proxies as per the ES2015 spec (I think it's using the old proxy API):

➜  ~  node -v
v5.8.0
➜  ~  node -p process.versions.v8
4.6.85.31
➜  ~

V8 4.9 does support ES2015 proxies, but it was only released in late Jan, so I'm not sure when it'll turn up with Node.

If you wanted to stick with Node for the tests right now, one option could be to use something like harmony-reflect, which patches pre V8 4.9 proxies to be ES2015 compliant?

@nmn
Copy link

nmn commented Nov 10, 2016

Proxies are everywhere now, I'll take a stab at this. Would love some help getting started though. I'm not sure exactly how the proxy works right now.

@agoldis
Copy link

agoldis commented Apr 29, 2017

Hello 😃

I would be happy to understand what's current status of the issue...

@wkwiatek, is https://github.com/gaearon/react-proxy/tree/use-proxies relevant (looks like it is)? I have started to work on your branch by fixing tests, but i am not sure my effort is worthy.

How does this issue plays together with the recent changes, e.g. create-react-class?

@wkwiatek
Copy link
Collaborator

Hi @agoldis

I've started evaluating proxies and it turned out that it's actually much bigger topic than I though and simply have no time to push it forward to the state that I wanted (and I'm still very busy know).

The point is, it's totally rewritten. It's also a little bit different approach as I started to render components using Enzyme to get the desired output and propagate it to the existing proxy component (it's due to e.g. arrow functions lexical context).

If you're willing to push it forward, I'd be happy to walk you through the code and show you what's going on there. The most recent state should work when swapping components (even connected by Redux) but still needs a lot of testing and fixing tests.

@agoldis
Copy link

agoldis commented May 10, 2017

@wkwiatek Thank you for reply.
I think i've managed to understand your code, you've done an awesome job! 👏
I have done some changes, causing more tests to pass. Moreover I have switched to enzyme in tests as well...

There are some tests, thought, that i am not sure should be maintained - e.g. createClass syntax will be gone (there's a deprecation warning in the latest version of react), so half of the tests probably won't be needed...

That's why i am wondering whether it's important to keep backward compatibility and we can just move forward and make all the tests pass just with the modern non-createClass syntax.

Anyway's I'll start to open PR's with small code changes and fixed tests.

@wkwiatek
Copy link
Collaborator

I think we can just move forward and even prepare it to be React 16 only. People using RHL are rather early adopters, and in my opinion, the best recommendation for using RHL should be: it just works (even for a limited environment first) or says no.

@agoldis
Copy link

agoldis commented May 15, 2017

great, so I have opened #78 and #77 in order to sync my current progress, there're still a lot of tests to fix, but first i wanted to make sure i work in modern environment and use modern tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants