Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

private fields will break vue/mobx #227

Closed
cztomsik opened this issue Mar 4, 2019 · 29 comments
Closed

private fields will break vue/mobx #227

cztomsik opened this issue Mar 4, 2019 · 29 comments

Comments

@cztomsik
Copy link

cztomsik commented Mar 4, 2019

https://github.com/vuejs/rfcs/blob/class-api/active-rfcs/0000-class-api.md#usage-with-private-fields
image

@trotyl
Copy link

trotyl commented Mar 4, 2019

Duplicate of #106

@nicolo-ribaudo
Copy link
Member

Can't the Vue class return a proxy?

@Igmat
Copy link

Igmat commented Mar 4, 2019

Huh, my prediction was true.

In the end of this #158 (comment) I said (in few other places I also mentioned it):

Since, there are no privates right now, libraries that use such approach are already built/in-progress (and I'm an author of such library, same as @yyx990803 (Vue.js) and @EisenbergEffect (Aurelia)), and there are NO WAY to achieve same goal and I don't want to write in my readme:

NOTE: this library can't be used together with ESnext privates

I don't want to restrict otherwise normal intent of my users to encapsulate implementation details, only because somebody decided that encapsulation without brand-checking isn't perfect.

I bet that we'll see more issues of such type.

Can't the Vue class return a proxy?

@nicolo-ribaudo, I don't know internals of Vue, but I work on library with quite similar usage of Proxy and even though I ended up with returning proxy from constructor - it doesn't fit all my needs, so I guess that Vue has more restrictions (may be backward-compatibility?) and can't do that.

BTW, if you want I may create small repo with this pattern showcase and tests, so you won't need to check how Vue/MobX/Aurellia/MetaF works in order to understand our requirements to this proposal.

/cc @littledan, @ljharb, @erights

@rdking
Copy link

rdking commented Mar 5, 2019

@nicolo-ribaudo
Look at the general scenario, and you'll see why this is so problematic.
A Proxy is meant to give code external to an instance a way of monitoring/manipulating/protecting properties of that instance. So suppose, Vue returns a proxy as you say. Unless it is created by extending another class or function that returns a new proxy object, Vue itself would not be able to have private members. For the sake of argument, let's suppose it doesn't. Vue would have to be absolutely certain that it doesn't call anything against targetfrom inside the Proxy handler. Unfortunately,target` doesn't have the private members. They were installed on the Proxy. This is where things get counter-intuitive really fast.

If you were thinking that this is ok because calling against the "receiver" is what should happen anyway since that was the class instance, there are many scenarios where doing so would result in overflowing the stack. Compound this with the reasonable expectation that anything written to the Proxy instance is applied to the internal instance unless interfered with by handler functions, and that Proxy cannot interfere with private-fields, and you've got a recipe for spontaneous failures and confusing misunderstandings.

Just my 2¢, but this is the kind of thing you expect to have happen when emotional and rational concerns outweigh logical concerns when designing a language or its features. Failure of a new feature to be compatible with all non-competing, non-conflicting existing features constitutes a design flaw. Allowing such a flaw to further infect post-facto features in perpetuity just because it's already there is just adding insult to injury.

@littledan
Copy link
Member

I've reached out to the Vue maintainers to see if we can address this issue along the lines @nicolo-ribaudo suggested.

@yyx990803
Copy link

Turns out it's working for us! Thanks to @nicolo-ribaudo for the idea!

@rdking
Copy link

rdking commented Mar 5, 2019

@littledan Is there any particular impact to the proposal should a major framework be found for which no such simple solution is applicable?

@EisenbergEffect
Copy link

As I understand it, that technique won't work for Aurelia since we're based on vanilla JS component classes. The component class is fully under the control of the developer. We don't (and won't) require a base class, as it's antithetical to our software design philosophy.

We can provide some documentation and potential workarounds for our community. However, I still feel that the private fields/proxy integration story is flawed.

@Jamesernator
Copy link

I'm still hopeful we can get a [[Target]] proxy trap as was suggested in one of the other issues somewhere. It worked something like this:

class Foo {
  #bar
  constructor() {
    this.#bar = 12
  }

  getBar() {
    return this.#bar
  }
}

const p = new Proxy(new Foo(), {
  target(target) {
    return target // By default target is the Proxy itself
  },
})

p.getBar() // 12, just works

From what I recall the way it works is simply that target() can return the object where private fields should be accessed from rather than just always the same target (and it could potentially extend to some internal slots too?).

@ljharb
Copy link
Member

ljharb commented Mar 11, 2019

I would expect it to work for all internal slots if such a thing existed.

@EisenbergEffect
Copy link

Was there some reason that we couldn't do this and make this enhancement to Proxy a necessary part of the private fields spec?

@ljharb
Copy link
Member

ljharb commented Mar 11, 2019

It’s a preexisting problem. It makes more sense to solve it in parallel.

@EisenbergEffect
Copy link

My concern is browsers implementing private fields without the associated Proxy feature to prevent runtime errors. It seems that if private fields cause the issue with proxies, then the Proxy enhancement should be implemented along with private fields by browser manufacturers.

@ljharb
Copy link
Member

ljharb commented Mar 11, 2019

@EisenbergEffect user code can cause the same issue right now by using WeakMaps, closures, and/or by extending builtins with internal slots. Private fields are just a new manifestation of a pre-existing issue with certain Proxy usage patterns.

@littledan
Copy link
Member

I hope to find time soon to investigate the Aurelia case further. Sorry for my delay here.

@bigopon
Copy link

bigopon commented Mar 12, 2019

For:

Private fields are just a new manifestation of a pre-existing issue with certain Proxy usage patterns.

This has been mentioned and argued several times, and probably most recently is by @jods4 at
#106 (comment)

Cases where proxies fail
Please stop bringing up situations where proxies are broken (primitives, etc).

Yes, we acknowledge from the very start of this thread that proxies have known quirks and limitations.
It is not a justification to add more.

Proxies work well enough in many situations. They have practical uses and can be found in existing JS code.

What you are proposing here will be a much worse limitation of proxies than any existing today.
It will strongly reduce the usefulness of proxies. Situations where they are used today will become impossible.

@rdking
Copy link

rdking commented Mar 12, 2019

@ljharb

@EisenbergEffect user code can cause the same issue right now by using WeakMaps, closures, and/or by extending builtins with internal slots. Private fields are just a new manifestation of a pre-existing issue with certain Proxy usage patterns.

Please understand that unlike with private fields, the issues in question when using WeakMaps and closures is perfectly resolvable in user-land code. However, there is no possible solution in user-land code when dealing with private fields. Even Babel's implementation of private fields can be made to work where native private fields will break.

@Igmat
Copy link

Igmat commented Mar 15, 2019

I hope to find time soon to investigate the Aurelia case further. Sorry for my delay here.

@littledan, in order to simplify your understanding of Aurelia (and other library authors like me) issue, I've created very minimal implementation of Proxy use-case that we use (@EisenbergEffect correct me, if I'm mistaken) here: https://github.com/Igmat/reactive-properties.

If you run tests, you'll see that it works in 2 from 3 cases:
✓ Class without any privates
✓ Class with Symbol.private
✘ Class with ES privates

If you have any question regarding Reactive Properties pattern or code/tests there - I would love to answer.
If you'll find a way to make that code work with class-fields-proposal in current state without changing code from emulate-third-party-modules, this issue will be solved entirely.

@Igmat
Copy link

Igmat commented Mar 15, 2019

WeakMaps, closures, and/or by extending builtins with internal slots

@ljharb, you missed that there are normal workarounds:

  1. WeakMaps - it's VERY rare case. I've never seen WeakMap usage for storing private data before joining discussions in this repo, so major library authors generally ignore this case, but solution is very easy - WeakMap could be monkey-patched to work with Proxies as they were targets. As far as I remember, @rdking has even shown the code for such patch.
  2. closures - hmm, I can't even imagine how closures may break reactive proxy use-case, could you clarify this please?
  3. extending builtins - I have never seen such code in the wild. But, still it'll just be special-cased in reactive proxy - since all built-ins are known beforehand it's not a big deal to handle them in special way.

Why do you repeat this again and again? We know this very limited number of cases and there are KNOWN WAYS how to handle them.
But there are NO WAY to handle privates from this proposal for reactive proxy use case.

P.S.

Don't forget that scale is essential, and amount of breaking WeakMap/builtin usages isn't even comparable to amount of future encapsulation usages in the wild.

@littledan
Copy link
Member

@Igmat, I know that the Aurelia issue exists. What I don't yet understand is all the context that led to its different design decisions.

@EisenbergEffect
Copy link

One of the most important things that Aurelia values is unobtrusveness. We hold a strong belief that your code should be at the center of your app, rather than the framework. So, we've worked very hard to not require things like a base class for components or decorators on observable properties. These would couple core application code to the framework, which we see as bad for businesses in the long run. The context for that is many, many years of building many apps for many companies and seeing the negative effects of frameworks and libraries that do not take that approach.

@rdking
Copy link

rdking commented Mar 18, 2019

@littledan Just out of curiosity, why is "the context that led to its different design decisions" at all relevant? Isn't the reality that different design decisions were made at all reason enough to add value to the fact that this proposal allows Proxy to be an unsolvable problem? Aurelia isn't the only code that is going to suffer over this completely impassable issue.

Another thing: providing encapsulation is great. However, what good is it when it comes at the cost of functionality that is arguably considered even more useful? Encapsulation vs reliable inheritance (since both base class and derived class accessors could break)? Encapsulation vs monitoring (since non-Membrane like Proxy usage will break)? Encapsulation is not meant to be pitted against other non-competing paradigms. Yet that's precisely what this proposal offers. It isn't that there isn't another approach that doesn't make these kinds of technical trade-offs, but rather that non-technical points of interest were deemed more valuable. Technical problems like this with no reasonable solution are the result of that evaluation.

@littledan
Copy link
Member

I think we're running into core Proxy design decisions, which made it not well-suited to usage patterns like this. If you expect to be able to wrap something in a Proxy from the outside and make that Proxy not unwrap to the target when calling methods, various things will break. This isn't just WeakMaps, but even something as simple as ===. What I'd like to understand from the context is if there are other ways we could solve the problem that Aurelia is trying to solve.

@Igmat
Copy link

Igmat commented Mar 18, 2019

but even something as simple as ===

This problem is also known, but in most cases it doesn't affect anything, since library authors just enforce that targets interacts in one space and proxies in another one, so proxy === target just doesn't happen.

is if there are other ways we could solve the problem that Aurelia is trying to solve.

Just check repo I mentioned before - there are no other ways to solve this problem, unless you provide totally new API other then Proxy for this pattern.

@rdking
Copy link

rdking commented Mar 18, 2019

@littledan Sure, there are issues with the core design of Proxy. Fairly large ones in fact. However, in current ES, every one of those issues is either irrelevant in most cases or resolvable using user-land code. This is the point everyone is trying to get through to you. What is currently solvable becomes conclusively unsolvable with no work arounds possible given the introduction of private fields.

The only work around is not to use private fields. However, that becomes problematic because libraries like Aurelia, and so much other code, are designed to interact with code outside of the control of those libraries. If an object containing private fields gets wrapped in a non-membrane-like proxy, access to private fields will fail. Full Stop.

If there was a work around for this that allowed for Proxy and private fields to be used together, I'm sure you wouldn't be receiving nearly as much push back on this issue. It might be a good idea to be more open to doing at least one of following 3 things:

  • reconsider the design of private fields with regard to Proxy
  • consider fixing Proxy's design a pre-requisite of private fields
  • consider an entirely different approach to private fields

@littledan
Copy link
Member

Closing as a duplicate of #106.

@cztomsik
Copy link
Author

@yyx990803

Turns out it's working for us! Thanks to @nicolo-ribaudo for the idea!

Maybe I'm missing something but:

  • have a private var
  • have a public method which accesses it
  • call that public method from a proxy (which is what vue does)
  • private violation

@nicolo-ribaudo
Copy link
Member

Can you share an example?

@cztomsik
Copy link
Author

okay, so this was my misunderstanding, and apparantely, it works:

class Counter {
  #count = 0

  inc() {
    this.#count++
  }

  peek() {
    return this.#count
  }
}

const c = new Counter()

// this is fine
c.inc()
console.log(c.peek())

// fine too
const p = new Proxy(c, {
  get(target, prop) {
    const res = target[prop]

    if (typeof res === 'function') {
      return (...args) => {
        console.log(`calling ${prop}`)
        return res.apply(target, args)
      }
    }

    return res
  }
})
p.inc()
console.log(p.peek())

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

No branches or pull requests