tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

private fields will break vue/mobx #227

Closed cztomsik closed 5 years ago

cztomsik commented 5 years ago

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

trotyl commented 5 years ago

Duplicate of https://github.com/tc39/proposal-class-fields/issues/106

nicolo-ribaudo commented 5 years ago

Can't the Vue class return a proxy?

Igmat commented 5 years ago

Huh, my prediction was true.

In the end of this https://github.com/tc39/proposal-class-fields/issues/158#issuecomment-432809666 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 commented 5 years ago

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

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

yyx990803 commented 5 years ago

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

rdking commented 5 years ago

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

EisenbergEffect commented 5 years ago

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

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

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

EisenbergEffect commented 5 years ago

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

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

EisenbergEffect commented 5 years ago

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

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

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

bigopon commented 5 years ago

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 https://github.com/tc39/proposal-class-fields/issues/106#issuecomment-400608926

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

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

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

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

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

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

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

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

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

@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:

littledan commented 5 years ago

Closing as a duplicate of #106.

cztomsik commented 5 years ago

@yyx990803

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

Maybe I'm missing something but:

nicolo-ribaudo commented 5 years ago

Can you share an example?

cztomsik commented 5 years ago

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())