gaearon / react-proxy

Proxies React components without unmounting or losing their state
459 stars 50 forks source link

Explore using ES2015 Proxies #40

Open gaearon opened 8 years ago

gaearon commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

: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 commented 8 years ago

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

ghost commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago
> 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 commented 8 years ago

http://stackoverflow.com/a/10666902/458193

dralletje commented 8 years ago

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

ghost commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

ghost commented 8 years ago

A PR of the browser tests or something else?

gaearon commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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