Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

emplace cannot be correctly and incrementally polyfilled #51

Closed
conartist6 opened this issue Nov 25, 2022 · 24 comments
Closed

emplace cannot be correctly and incrementally polyfilled #51

conartist6 opened this issue Nov 25, 2022 · 24 comments

Comments

@conartist6
Copy link

The problem is that since emplace is defined not to call get and set, it can only be correctly polyfilled when completely reimplementing Map from scratch, as core-js does. In other words, it is impossible to build the functionality correctly by extending the Map prototype because you would not have the requisite privileged access to the underlying data structure.

My impression is that that would make this one of those features that we can all use in ~5 years when everyone is finally using engines that natively support it.

@ljharb
Copy link
Member

ljharb commented Nov 26, 2022

What makes it a Map imo isn’t the api, it’s the internal slots - thus, if your subclass isn’t utilizing them, it’s not really a Map instance, it’s just a Map-like.

iow, there’s no need to subclass Map at all for such a class.

@conartist6
Copy link
Author

conartist6 commented Nov 26, 2022

I agree, but my question is: how will you (you personally probably) write a shim? You won't have access to the internal slots you need to avoid calling get and set.

@zloirock
Copy link
Contributor

zloirock commented Nov 26, 2022

const has = Function.call.bind(Map.prototype.has);
const get = Function.call.bind(Map.prototype.get);
const set = Function.call.bind(Map.prototype.set);

function emplace(key, handler) {
  const M = this;
  if (has(M, key)) { // incl. RequireInternalSlot(M, [[MapData]])
    let value = get(M, key);
    if ('update' in handler) {
      value = handler.update(value, key, M);
      set(map, key, value);
    } return value;
  }
  const inserted = handler.insert(key, M);
  set(M, key, inserted);
  return inserted;
}

Object.defineProperty(Map.prototype, 'emplace', { writable: true, configurable: true, value: emplace });

@conartist6
Copy link
Author

conartist6 commented Nov 26, 2022

Wouldn't it have to be:

class MapWithEmplace {
  constructor(entries) {
    this.#nativeMap = new NativeMap(entries);
  }
  get(key) {
    this.#nativeMap.get(key);
  }
   // ... 
  emplace(key, handler) {
    const M = this.#nativeMap;
    if (has(M, key)) {
      let value = get(M, key);
      if ('update' in handler) {
        value = handler.update(value, key, M);
        set(map, key, value);
      } return value;
    }
    const inserted = handler.insert(key, M);
    set(M, key, inserted);
    return inserted;
  }
}

@zloirock
Copy link
Contributor

Wouldn't it have to be

Why?


In this case, the problem is not in writing shims - the problem is in subclassing and generic behavior. But TC39 kills it already for a long time.

@conartist6
Copy link
Author

conartist6 commented Nov 26, 2022

The proposal currently says (it should state this in clearer language) that the implementation of map.emplace must not call map.get or map.set. Yours does. You can make it not do that by using internal slots, but you can only have access to internal slots because you replaced native Map with something else (global.Map = ...).

@zloirock
Copy link
Contributor

It does not call map.get or map.set, you could remove them at all. It works completely by the spec and interacts with internal slots.

@conartist6
Copy link
Author

How does it interact with the internal slots? If I write new Map right now, I certainly can't get access to its internal slots.

@zloirock
Copy link
Contributor

zloirock commented Nov 26, 2022

Please, take a look at the polyfill source above.

@conartist6
Copy link
Author

I don't feel at all confident that you are hearing what I am saying : (

The polyfill source you link implements Map.prototype.emplace in terms of Map.prototype.has etc, which AFAIK is wrong. Do you disagree? Did I misread?

@conartist6
Copy link
Author

See: #47

@zloirock
Copy link
Contributor

One more time - it does not call .get or .set on this, they are unbound. It has no any observable difference with direct interaction with internal slots. It works completely by the spec of this proposal.

@conartist6
Copy link
Author

How?? There's something going on here that's extremely obvious to you and not at all obvious to me

I know you'll have access to the internal slots of you've completely implemented Map from the ground up.

A shim however is not a ground up implementation, but only an implementation of the part of the functionality that is missing.

@zloirock
Copy link
Contributor

Please, write any example that will not work correctly by the spec with the polyfill above.

@conartist6
Copy link
Author

What is RequireInternalSlot?

@zloirock
Copy link
Contributor

zloirock commented Nov 26, 2022

has checks it.

@conartist6
Copy link
Author

I'm going to take 24 hours to cool off from this. Thanks.

@conartist6
Copy link
Author

My question is still utterly unanswered.

@ljharb
Copy link
Member

ljharb commented Nov 26, 2022

@conartist6 call-binding is how you unobservably base a polyfill’s functionality off of existing builtins, but either way, polyfillability is not a consideration of spec proposals.

@zloirock
Copy link
Contributor

but either way, polyfillability is not a consideration of spec proposals

Why do you think so? Polyfill is something that significantly improves user experience and allows you to start using a feature a few years earlier. If polyfillability can be implemented at a low cost, it should be implemented.

@conartist6
Copy link
Author

@ljharb Thanks for the succinct clarification. I've never needed to use that pattern so I didn't recognize it.

@ljharb
Copy link
Member

ljharb commented Nov 26, 2022

@zloirock it’s not that it’s my preference, it’s a statement of fact about the committee’s historical priorities. Here’s not the place to debate it.

@zloirock
Copy link
Contributor

zloirock commented Nov 26, 2022

@ljharb this is a thread about polyfillability, so it's the correct place for this discussion. Historical priorities show that almost all that can be made polyfillable without any significant issues is made polyfillable - and this is something that should be defined in TC39 rules because someone is trying to change it.

@conartist6
Copy link
Author

If you want to make a rule it seems reasonable to start a thread in a place where the people who will be affected (and who can effect rule changes) would see it.

This is a thread based on a technical misunderstanding, and I am closing it.

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

No branches or pull requests

3 participants