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

Alternative API (Inverted) #106

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

martinheidegger
Copy link

Adjusting the proposal to use the alternative API proposed in: #105

Note: This PR is currently only to show how the API would change to give a sense how it would feel different to eventually use it.

First commit of inverting the API as proposed in tc39#105
In the example for transitive task attribution "this" was missing.
@nicolo-ribaudo
Copy link
Member

This API can be implemented as a library on top of the current version of the proposal, right?

// my-library.js

const variables = new WeakMap();

const runWith = AsyncContext.Variable.prototype.run;

export function run(values, cb) {
  Object.entries(values).reduce(([symbol, value], nextCb) => {
    if (!variables.has(symbol)) variables.set(symbol, new AsyncContext.Variable());
    return runWith.bind(variables.get(symbol), value, nextCb);
  })();
}

export function get(symbol) {
  return variables.get(symbol)?.get();
}

@martinheidegger
Copy link
Author

Yes. The API is feature equivalent.

@martinheidegger
Copy link
Author

martinheidegger commented Oct 14, 2024

Amendment: While feature equivalent, my assumption is that the proposed API consumes less resources.

@martinheidegger
Copy link
Author

martinheidegger commented Oct 14, 2024

The current API could also be implemented as library on top of the API in the proposal. (I havn't tested the code below but should work)

class Variable {
  constructor (options) {
    this.#options = options
    this.#symbol = Symbol()
  }
  get name() {
    return this.#options?.name
  }
  get() {
    return AsyncContext.get(this.#symbol) ?? this.#options?.defaultValue
  }
  run(value, fn) {
    return AsyncContext.run({ [this.#symbol]: value }, fn)
  }
}

@legendecas
Copy link
Member

legendecas commented Nov 5, 2024

#101 expanded on a future extention like using _ = asyncVar.withValue("main") to allow mutating an AsyncContext.Variable without nesting callbacks.

This alternative "map" like API allowing the snapshot being accessed with a string key voilates the OCAP constraint. It introduces a new global state where the value could be accessible merely with a string name. On the other hand, the AsyncContext.Variable API only allows accesses from who has access to the AsyncContext.Variable instance.

@martinheidegger
Copy link
Author

Yes, String access is global and allows a global state. To me that was more of a feature that a bug. However, if this is a concern, strings and Symbols that are equals to Symbol.for(...) Symbols could be null operations, it could be possible to pass them but when requesting them they would throw an error or return null.

@mhofman
Copy link
Member

mhofman commented Nov 5, 2024

Yes, String access is global and allows a global state. To me that was more of a feature that a bug. However, if this is a concern, strings and Symbols that are equals to Symbol.for(...) Symbols could be null operations, it could be possible to pass them but when requesting them they would throw an error or return null.

I'm not sure I understood this right, but effectively anything that is forgeable (string, registered or well-known symbol, etc) is not ok from an ocaps perspective to use as a key in global registry as it creates an observable global mutable state. A global registry is only possibly acceptable if keyed by something that can be used as a WeakMap key. Even then there are further requirements to avoid this registry to be used as communication channel.

@martinheidegger
Copy link
Author

I am fine with the restriction to ignore all but non-wellknown symbols as keys.

@mhofman
Copy link
Member

mhofman commented Nov 7, 2024

I am fine with the restriction to ignore all but non-wellknown symbols as keys.

I think there's still a misunderstanding somewhere. To prevent observable mutable global state, the only kinds of symbols that may be acceptable would be unique symbols (as created by Symbol()). Anything else is a non starter.

@martinheidegger
Copy link
Author

martinheidegger commented Nov 7, 2024

I am sorry, I should have elaborated:

const string = "a-key"
const num = 1
const bool = true
const wellKnownSymbol = Symbol.for('a-key')
const secretSymbol = Symbol()

AsyncContext.run({
  [string]: 'a',
  [num]: 'b',
  [bool]: 'c',
  [wellKnownSymbol]: 'd',
  [secretSymbol]: 'e',
}, () => {
  AsyncContext.get(string) === undefined;
  AsyncContext.get(num) === undefined;
  AsyncContext.get(bool) === undefined;
  AsyncContext.get(wellKnownSymbol) === undefined;
  AsyncContext.get(secretSymbol) === 'e';
})

It could also accept WeakMap objects, but I don't see a lot of value for it.

const obj = {}
const map = new WeakMap([[obj, 'f']])
AsyncContext.run(map, () => {
  AsyncContext.get(obj) === 'f'
})

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.

4 participants