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

Support top-level-await #27

Open
kriskowal opened this issue Oct 22, 2020 · 11 comments
Open

Support top-level-await #27

kriskowal opened this issue Oct 22, 2020 · 11 comments

Comments

@kriskowal
Copy link
Member

kriskowal commented Oct 22, 2020

As proposed at the time of writing, importNow returns an exotic module exports namespace object and throws an exception if the specified module or any of its transitive dependencies have not yet loaded. The import method returns a promise for exports because it may need to load the dependencies using the asynchronous loader hook #26.

Top-level-await introduces the possibility that importNow would need to do asynchronous to fully initialize the transitive dependencies. To that end, we have some options to bring the compartment proposal in alignment with top-level-await.

  1. importNow continues to work as-is for module graphs that lack any top-level-await.
    1. importNow returns a promise if any dependency uses top-level-await.
    2. importNow throws an exception if any dependency uses top-level-await.
  2. Remove importNow and rely on the import method to serve in its stead regardless of whether any dependency uses top-level-await.

The first option (and two suboptions) introduce a hazard in the face of changing dependencies. If any of a module’s transitive dependencies add a first top-level-await, the signature or behavior of importNow changes for that module in a way that breaks existing code. If we choose to keep importNow, we would need to socialize that adding a top-level-await to a published module is a breaking change and that it would require a major version number bump and a migration to be done safely.

Supporting top-level-await also requires ensuring that the compartment proposal meshes with the existing specified rules for initializing module subgraphs. I expect that this will require no adjustments to the interface as defined today.

Third-party module support #36 creates a case for keeping importNow, since third party module initializers can expect that their dependencies have already executed.

See #31 for a fresh attempt to capture this issue description, closed as duplicate.

@Jack-Works
Copy link
Member

1.1. importNow throws an exception if any dependency uses top-level-await.

Is it possible to make those two APIs into one so the developer can decide if they want to throw when it containing a top-level-await?

For example:

const result = compartment.import(str)
if (result.type === 'async') {
    console.error('Promise:', result.promise)
    throw new Error('Cannot contain top-level-await')
} else {
    assert(result.type === 'sync')
    console.log(result.module)
}

@kriskowal
Copy link
Member Author

@Jack-Works That is certainly an option, and it suffers the same hazard. Code that failed to account for both cases would break. This would likely lead to a great deal of unnecessary ceremony and breakage.

@devsnek
Copy link
Member

devsnek commented Oct 22, 2020

There was a significant amount of effort put into TLA to ensure it would not be breaking for a module's dependencies to use it. It would be unfortunate to expose it here.

@Jack-Works
Copy link
Member

If we do not want to expose it, we need to disable sync import totally 👀

@coder-mike
Copy link

Is it correct to say that application code can avoid being a victim of the breaking change by just always using import instead of importNow?

@kriskowal
Copy link
Member Author

Is it correct to say that application code can avoid being a victim of the breaking change by just always using import instead of importNow?

Yes. Regardless of these two options, import would always work.

@hax
Copy link
Member

hax commented Oct 29, 2020

There was a significant amount of effort put into TLA to ensure it would not be breaking for a module's dependencies to use it.

@devsnek

How? As my last reading of TLA proposal, any module introducing TLA will be a breaking change and need to update its major semver.

@devsnek
Copy link
Member

devsnek commented Oct 29, 2020

@hax that is incorrect. it creates a graph with async edges, from the perspective of esm code it doesn't change at all, and since import() already returns a promise, it isn't breaking.

@Jack-Works
Copy link
Member

@devsnek what if code runs like this?

<script type="module">
import { setup } './something.js'
setup()
</script>
<script type="module">
assert(already_setup)
</script>

And one-day "something.js" is starting to use TLA?

@devsnek
Copy link
Member

devsnek commented Oct 29, 2020

@Jack-Works it is unclear to me what that code does (is it using globals??), and I don't know the rules of evaluation for script type=module tags. but the above point relates to using await inside a module not being a breaking change within a single graph. your example uses two graphs which race each other, which seems dangerous even without using tla.

@hax
Copy link
Member

hax commented Jan 19, 2021

your example uses two graphs which race each other, which seems dangerous even without using tla.

Scripts of <script type=module> (without async attr) are ensure to executed one by one.

Unfortunately TLA will break that. So introducing TLA in any modules which directly or indirectly referenced by the script files eventually become a breaking change and could cause random bugs which will be very hard to locate.

Of coz, someone argues that two <script type=module> should not share anything, but if that, people should use <script type=module async>, so this actually make the original design of <script type=module> meaningless.

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

5 participants