-
Notifications
You must be signed in to change notification settings - Fork 41
Use a class instead of creating a new object with closures each time #2
Comments
The proposal suggests that the factory should go on the System object, but I still prefer |
I'll add this to the open questions and plan to add a section to the document to describe the motivation and whatever change/resolution we come to here.
We would also prefer to use the standard pattern. After some Here's the issue: Say Bob does not have access to System.WeakRef const wr = new System.WeakRef(....);
bob(wr); if the caller judges it safe enough to enable Bob to observe when this const WR = Object.getPrototypeOf(wr).constructor; to obtain the weak reference constructor. There are simple variations However, regarding:
that was definitely not the intent; whatever the final there should be |
Can't systems that want to restrict access... just restrict access? Virtualize or null out |
I also think we should put this on the global, not on a namespace object. There is no practical difference and the former fits better with the language. |
That's an example of "it's mostly the class pattern but not quite" that can be confusing. For example, if the constructor is null'd, is "new WeakRef" still well-defined? (That's a rhetorical question, btw.) So further exploration around other intrinsics might yield an existing object to match the specifics of (@erights suggested %MapIterator% as a possibility). |
That sounds like an acceptable sacrifice for such systems to make in order to allow the rest of the ecosystem to interface with this through standard JavaScript patterns. |
Yes, in ES6 there are several unnamed intrinsics, the ones listed in the "Intrinsic Name" column of https://tc39.github.io/ecma262/#sec-well-known-intrinsic-objects but where their "Global Name" is blank. Many of these are prototypes for which there is no corresponding constructor, thereby establishing precedent as a pattern for providing constructor-less vtables of methods for instances to inherit from. Let us indeed use %MapIteratorPrototype% https://tc39.github.io/ecma262/#sec-%mapiteratorprototype%-object as an example. (I think this is what Dean meant rather than %MapIterator%.) new Map()[Symbol.iterator]().constructor === Object;
true Likewise, we should revise the weakref proposal to be more ecma-spec-like by:
This does raise another cross realm issue we need to be careful of: what if a %WeakRefPrototype%.get method in one realm is applied to a weakref instance from another realm? Unlike cross-realm weakly pointing, this is a potential cross realm leakage issue that I have not yet thought about. My first reaction is this one is ok, but I am not yet convinced there's no security danger lurking there. Nevertheless, this new cross-realm question should not block the intra-realm transition from the current spec language to one in terms of %WeakRefPrototype%. |
Starting with the default loader,
If anything like the current zones proposal were to go forward, many things would have to change. But anything like its "Zone" object would also go on System. getSourceMap is arguable, as no authority is provided, so I'm not sure System is the best place for it. However, since getStack must be on System, and since a source remapping needs to virtualize these together, System may be the best place. Leaving aside getSourceMap, the general issue is closely analogous to the difference between user-mode and system mode instructions. Anyone who has not yet read the classic See the discussion at whatwg/loader#34 which concluded that the Loader constructor should be completely authority free -- all of a loader's ability to cause i/o should come from its constructor parameters -- and so the Loader constructor should go on |
I don't agree with this general direction. All of the proposals you mention are still nonstandard and one of my personal criteria for moving most of them forward will involve moving them to other more conventional objects (global, Error, etc.). I don't think this idea has as much support as you think it does. And I think that breaking the X.prototype.constructor === X link (for APIs where X is meaningful, like WeakRef but unlike %MapIteratorPrototype%) should be confined to environments that need to do so, and not affect the language spec. |
I updated the proposal to use a prototype. That seems like several steps in the right direction, regardless. |
I'm with Domenic here. I don't think we should be turning |
Agree as well. |
At tc39/ecmascript_simd#323 I write:
Once we understand what that looks like in the absence of security constraints, let's revisit it for weakrefs. If it makes sense, I would certainly prefer to see this rooted in a module rather than a global. |
I too would prefer a module, assuming it can address the security requirements. Weak references are intended for use by library and framework creators, and shouldn't appear in most application code. Thus, even independent of security concerns, they shouldn't be in a global namespace. |
At the March meeting, there seemed to be a positive reaction to the idea of exposing the API via a WeakRef constructor. The challenge with that approach, if I remember correctly, is that we may not want to expose the WeakMap constructor to code that is given a reference to a WeakRef instance. As a solution to that problem, we briefly discussed the idea of a meta API on superclasses which informs subclasses that they should not attach a "constructor" property to the newly created prototype object. Has any more thought been given to this issue? It seems like it may be a blocker for the surface API of this proposal. |
I had an informative side conversation at that tc39 meeting, but I'm not sure with whom. The suggestion which emerged is: Introduce a new symbol, say An annotation on Open questions:
|
The current proposal is based on a class. |
ES prefers using classes to encapsulate methods that are reused, instead of re-creating new closures every time a given type of object is created. So, it should be
new WeakRef(...)
withWeakRef.prototype.get
andWeakRef.prototype.clear()
.The text was updated successfully, but these errors were encountered: