Skip to content

Conversation

@marshall007
Copy link
Contributor

See #94

@marshall007
Copy link
Contributor Author

The remaining test failures are also the result of various instanceof Term. I'll try to figure out a better solution than doing the __proxy check everywhere before pushing a fix.

@neumino
Copy link
Owner

neumino commented Jun 8, 2015

What's the cost in terms of performance for Proxy? And especially the harmony-reflect?
If it's not a trivial change, we may want an option to turn it on/off instead. I haven't tested it yet :)

@neumino
Copy link
Owner

neumino commented Jun 8, 2015

The reason why I asked a bit about performance is just because Term is called at least once for every methods.

Also, do we need the npm package? Or is Proxy not supported on Node for now?

@marshall007
Copy link
Contributor Author

@neumino sorry, I've been out on vacation. The harmony-reflect dependency was so I could use the Reflect API which isn't currently supported, but I think we could get around this if you'd prefer to not have it.

No idea what the performance impact is, I'll try to look into it. I think we could do feature detection and only proxy Term if Proxy is defined as a global (i.e. the harmony_proxies flag is enabled).

@marshall007
Copy link
Contributor Author

I've tried manipulating the term instance in various ways in the constructor, but I still can't get instanceof Term to work properly. I'm thinking instead of always proxying Term we could just assign a proxied object to term.$ (or something). This would have several advantages:

  1. Any performance impact would be strictly opt-in.
  2. It would be obvious when you're using the proxy (which addresses your point in Cannot create connection for the pool while using thinky #93 about being explicit).
  3. The instanceof checks would work again and we wouldn't have to rely on internal flags like __proxy for type checking.

If we did it this way the syntax would instead look like this:

r.row.$.foo.bar === r.row('foo')('bar')

@marshall007
Copy link
Contributor Author

@neumino I spent some more time on this and made decent progress. I have all the tests passing with the exception of ones related to toJSON, which I had to comment out for now.

Also, I think the prototype inheritance issue is actually just a bug in the harmony-reflect polyfill and has nothing to do with the behavior of Proxy itself. I'm working on submitting a patch for this which I think will fix both instanceof and the problem with toJSON / JSON.stringify().

@marshall007
Copy link
Contributor Author

Closing in favor of #178.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants