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

Re-throw evaluation errors on deferred namespace property access #43

Merged

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Apr 24, 2024

This commit changes the behavior of namespaces obtained through import defer to re-throw evaluation errors even if the module is fully evaluated:

import defer * as ns from "module-that-throws";
try { ns.foo } catch { console.log("Error!") }
try { ns.foo } catch { console.log("Error!") }

The code above is now guaranteed to print "Error!" twice, and it doesn't depend on whether other modules already evaluated "module-that-throws".

This is different from the existing behavior of namespace objects, so we have to use a different namespace object for deferred import:

import defer * as ns1 from "module-that-throws";
import * as ns2 from "module-that-throws";

Promise.resolve().then(() => {
  try { ns1.foo } catch { console.log("Error1!") }
  try { ns2.foo } catch { console.log("Error2!") }
});

Assuming that this module's body is evaluated due to a cycle (module-that-throws imports it, and module-that-throws is the entry point), the code above will print "Error1!" and not "Error2!". This means that ns1 !== ns2. Namespace objects are still cached, so if you import defer the same module twice you will get the same namespace object.

Thanks to this change, we can now also make "trying to evaluate a deferred module in a cycle" an error, rather than silently skipping its evaluation as done in #39. This ensures that a binding cannot be accessed during evaluation to then disappear as soon as the module is done evaluating with an error.

Fixes #41 cc @guybedford

This commit changes the behavior of namespaces obtained through
`import defer` to re-throw evaluation errors even if the module
is fully evaluated:

```js
import defer * as ns from "module-that-throws";
try { ns.foo } catch { console.log("Error!") }
try { ns.foo } catch { console.log("Error!") }
```

The code above is now guaranteed to print `"Error!"` twice,
and it doesn't depend on whether other modules already evaluated
`"module-that-throws"`.

This is different from the existing behavior of namespace objects,
so we have to use a _different_ namespace object for deferred import:

```js
import defer * as ns1 from "module-that-throws";
import * as ns2 from "module-that-throws";

Promise.resolve().then(() => {
  try { ns1.foo } catch { console.log("Error1!") }
  try { ns2.foo } catch { console.log("Error2!") }
});
```

Assuming that this module's body is evaluated due to
a cycle, the code above will print `"Error1!"` and not
`"Error2!"`. This means that `ns1` !== `ns2`.
Namespace objects are still cached, so if you `import defer`
the same module twice you will get the same namespace object.

Thanks to this change, we can now also make "trying to evaluate
a deferred module in a cycle" an error, rather than silently
skipping its evaluation as done in tc39#39. This ensures that a binding
cannot be accessed _during evaluation_ to then disappear as soon
as the module is done evaluating with an error.

Fixes tc39#41 cc @guybedford
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Nice, this is a much cleaner mental model for deferred evaluation error and cycle handling edge cases. I think we should go ahead with this change.

One possible optimization - in the case where a deferred module has already successfully executed, we could provide the normal namespace instead of the deferred namespace without there being any observable difference. This could possibly be a spec note or even specified.

Finally, do you have a path to implementation for the remaining TODO here in the bindinge resolution?

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated
<ins>an Object or ~empty~</ins>
</td>
<td>
<ins>The Module Namespace Object (<emu-xref href="#sec-module-namespace-objects"></emu-xref>) whose [[Deferred]] slot is **true**, if one has been created for this module.</ins>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Perhaps use the term requested as opposed to created.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied from the description of the non-deferred namespace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are not completely the same though because while for normal namespaces, the namespace being created is a matter of time, while for deferred namespaces, the namespace being created is a small percentage chance.

Some subtle wording difference in the description might help here.

@nicolo-ribaudo
Copy link
Member Author

@guybedford I updated the spec. It turns out the TODO I left is already handled, but I left it as a NOTE since it's not trivial to notice.

One possible optimization - in the case where a deferred module has already successfully executed, we could provide the normal namespace instead of the deferred namespace without there being any observable difference. This could possibly be a spec note or even specified.

Guy and I discussed this in the modules call last week -- this cannot be done because the identity of the two namespaces is observably different.

README.md Outdated Show resolved Hide resolved
nicolo-ribaudo and others added 2 commits May 14, 2024 09:56
Co-authored-by: Jack Works <5390719+Jack-Works@users.noreply.github.com>
@nicolo-ribaudo nicolo-ribaudo merged commit 52d40a5 into tc39:main May 14, 2024
@nicolo-ribaudo nicolo-ribaudo deleted the import-defer-namespace-rethrow-errors branch May 14, 2024 16:19
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request May 21, 2024
10 tasks
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.

Error behaviour details
3 participants