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

Add AsyncContext namespace and Snapshot and Variable #55

Merged
merged 11 commits into from
Jun 14, 2023
Merged

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 5, 2023

To distinguish the abstraction around the global state and local state, replace the built-in class AsyncContext with AsyncContext.Snapshot and AsyncContext.Variable.

Fixes: #44
Fixes: #36
Fixes: #21

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of using a namespace for the 2 classes? Something like: AsyncContext.Snapshot and AsyncContext.Local. I like that this

  • scopes the APIs to a unified place, making it easier to find the other related class (in docs and when playing around in dev tools)
  • Keeps the naming of "context", so that we can refer to all locals as the context and the individual locals as just local.
  • Might make loading the code easier for Moddable?

Other than that, some minor naming changes.

README.md Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

I didn't find precedence of constructors nested in a namespace in ecm262 (please correct me if there is one). I think it can be a good idea to do so, like WebAssembly.

@jridgewell
Copy link
Member

We'll have Temporal namespace soon, and we already have the Intl namespace (though that's technically ecma402).

@littledan
Copy link
Member

Aesthetic thoughts of mine, feel free to ignore:

  • I prefer to keep the constructors here ungrouped; that is a personal aesthetic judgement. This helps us avoid the question, "What is an AsyncContext?" (when there is no good answer--it's just a namespace).
  • I'd also prefer AsyncVariable over AsyncLocal (since locals stay within lexical scope, right?).
  • I think we should avoid using the word "run" for both AsyncLocal and AsyncSnapshot. Maybe for the latter, we could call the method restore?

I haven't reviewed the spec text yet, but don't wait on my review to land it.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

Fixed a few nits and renamed AsyncSnapshot.prototype.run to AsyncSnapshot.prototype.restore to avoid confusion with AsyncLocal.prototype.run.

spec.html Outdated Show resolved Hide resolved
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh, I forgot to hit the comment button on the review.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@legendecas legendecas changed the title Add AsyncSnapshot and AsyncLocal Add AsyncContext namespace and Snapshot and Variable Jun 13, 2023
@legendecas
Copy link
Member Author

Updated with grouping the classes Snapshot and Variable in the global namespace AsyncContext.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@legendecas
Copy link
Member Author

Reverted back to AsyncContext.Snapshot.prototype.run as @littledan brought up concerns from people on the name restore. restore can suggest to people that the global async context mapping is restored permanently.

@legendecas legendecas merged commit d269f3a into master Jun 14, 2023
@legendecas legendecas deleted the split branch June 14, 2023 03:33
@Qard
Copy link

Qard commented Jun 21, 2023

This split seems very weird to me. It feels unidiomatic to have a whole separate constructor available for snapshots rather than just a static method that returns a Snapshot type. The snapshot is meant to be a capture of what's in the stores so it seems intrinsically tied to the stores themselves. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants