Skip to content
This repository has been archived by the owner on Aug 29, 2021. It is now read-only.

Promise in Evaluate() Concrete Method makes no sense #31

Closed
GeorgNeis opened this issue Jul 10, 2018 · 6 comments
Closed

Promise in Evaluate() Concrete Method makes no sense #31

GeorgNeis opened this issue Jul 10, 2018 · 6 comments

Comments

@GeorgNeis
Copy link

Evaluate now returns a promise, but that promise is disconnected from InnerModuleEvaluation and is always resolved right before returning it.

@GeorgNeis GeorgNeis changed the title Promise in Evaluate( ) Concrete Method make no sense Promise in Evaluate() Concrete Method makes no sense Jul 10, 2018
@bergus
Copy link

bergus commented Jul 10, 2018

I think the whole evaluation [[Status]] and [[EvaluationError]] should be scrapped in favour of storing the promise as soon as the module is instantiated.

@guybedford
Copy link
Collaborator

@bergus agreed storing the promise sounds like the better approach, especially when dealing with overlapping subgraph execution cases as well.

@guybedford
Copy link
Collaborator

guybedford commented Aug 20, 2018

To add another note here while I've been working on this, the stored promise should be of the current execution only, and not including the dependency executions, so that overlapping top-level circular execution graphs don't have to wait unnecessarily on eachother (where there can be different subtree execution orderings).

@bergus
Copy link

bergus commented Aug 21, 2018

@guybedford Not sure whether we should disallow circles in asynchronously evaluated modules (i.e. with top-level await) altogether. I do like that circular dependencies between modules are allowed, but they're only good for declarative code. Having promise races influence the execution graph sounds hazardous. (To be fair, dynamic import() already sailed that ship)

@littledan
Copy link
Member

@GeorgNeis Yes, I agree that this is a problem. Do you think that guybedford#1 provides a working solution?

@littledan
Copy link
Member

This issue is fixed now that #33 has landed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants