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

Proposal: Nested import declarations #646

Closed
wants to merge 8 commits into from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 26, 2016

Stage: 0

Champion: Ben Newman (Meteor Development Group)

Specification: Work in progress; see attached commits.

Summary

This proposal argues for relaxing the current restriction that import declarations may appear only at the top level of a module.

Specifically, this proposal would allow import declarations that are

  1. nestable within functions and blocks, enabling multiple new and worthwhile import idioms;
  2. hoisted to the beginning of the enclosing scope; that is, declarative rather than imperative;
  3. lexically scoped to the enclosing block;
  4. synchronously evaluated, with clear and manageable consequences for runtime module fetching; and
  5. backwards compatible with the semantics of existing top-level import declarations.

At this time, no changes are proposed regarding the placement or semantics of export
declarations. In the opinion of the author, keeping all export declarations at the top
level remains important for many of the static properties of the ECMAScript module system.

Read the full proposal here.

benjamn added 8 commits July 23, 2016 20:00
Previously, CreateImportBinding was only supported for module Environment
Records. Supporting the CreateImportBinding method for declarative
Environment Records will be important for nested |ImportDeclaration|s,
since the declarative Environment Record for the nested block containing
the |ImportDeclaration| will need to allow indirect bindings, too.

Because declarative Environment Records may now have indirect bindings,
the GetBindingValue method of declarative Environment Records now fully
incorporates the logic of the GetBindingValue method of module Environment
Records, and so the module-specific method has been removed.
…ion.

This change means |ImportDeclaration|s are no longer grammatically
restricted to the top level of a module.

It also means |ImportDeclaration|s are no longer grammatically restricted
to |Module|s, and could potentially appear in |Script|s, which is a
problem to be addressed in a follow-up commit.
Note that this change allows functions and blocks to define bindings for
|ImportDeclaration|s, which was not previously possible.

After this change, the [[ImportEntries]] field of Module Record is no
longer used anywhere in this specification.
The top-level ImportEntries of a module are now extracted as part of the
CreateLexicalBindings operation, based on the top-level
LexicallyScopedDeclarations of the module, which now include
|ImportDeclaration| productions in addition to other lexically scoped
declarations like function/generator declarations and let/const variable
declarations.

Though it is possible that another specification might depend on this
field, the only relevant specification that I'm aware of is the WHATWG
Loader spec (https://whatwg.github.io/loader), and while it refers to the
Source Text Module Record in many places, it never makes use of the
[[ImportEntries]] field.

Moreover, I think we should remove this field now rather than allowing it
to persist, because any hypothetical auxiliary specification that relies
on [[ImportEntries]] is likely to be treating top-level import
declarations differently from nested import declarations, and thus runs
the risk of mishandling nested import declarations.
…ings.

Unless this note is wrong, we do not have to worry about import
declarations in eval strings, which is good news because it would be
impossible to determine statically what module identifiers are requested
by a dynamic string of code.
This change means nested modules will be evaluated at the beginning of
their enclosing blocks, if they have not already been evaluated.
@benjamn
Copy link
Member Author

benjamn commented Jul 26, 2016

As a personal note, my Monday evening flight from JFK to SEA for the July TC39 meeting was canceled, but I'll be there by this afternoon!

@pleath
Copy link

pleath commented Jul 28, 2016

On the subject of the impact on lazy/deferred parsing... You were proposing a solution that involved tokenizing the script to find the nested import statements. In my experience, it isn't possible to tokenize JS accurately without tracking expressions. OTOH, I don't see recognizing import statements in a lazy parse as a deal-breaker. The work required (for Chakra, anyway) would be on the order of the work we already do to recognize early errors, for instance.

@bterlson
Copy link
Member

For a feature like this I think there should be a tc39 repo for the proposal (rather than putting everything in a PR and tracking all issues in this one).

@benjamn
Copy link
Member Author

benjamn commented Jul 28, 2016

@bterlson ok, agreed, I will create a new repository once I've updated the proposal to accommodate the feedback from yesterday

@concavelenz
Copy link

I like this a lot (or something like it) because it makes it possible to
have reasonably safe circular references: something that I've been
struggling with to create a reasonable solution with ES6 modules

// parent

export class Parent {
newChild() {
import Child from 'child.js' // isn't evaluate until it is needed.
return new Child();
}
}

// child

import Parent from 'parent.js'

export class Child extends Parent {
newSibling() {

return new Child()

}
}

//

That is, with this it is possible to import 'parent' from another file
without causing 'child' to evaluate before "Parent" has been assigned a
value.

On Thu, Jul 28, 2016 at 12:37 PM, Ben Newman [email protected]
wrote:

@bterlson https://github.com/bterlson ok, agreed, I will create a new
repository once I've updated the proposal to accommodate the feedback from
yesterday


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#646 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABMDKhzcUszP0SoUrcH950H-c4PX7XYhks5qaQUAgaJpZM4JVcli
.

@shicks
Copy link

shicks commented Jul 29, 2016

As others have mentioned, I'd love to see this developed more fully in its own proposal repo.

But I wonder about how feasible the nested import is. For top-level imports, the loader can be reasonably efficient about pulling in all dependencies upfront. But for nested imports, having them return synchronously means an immediate blocking HTTP request before the method can complete, and that doesn't seem feasible. Maybe it could be allowed and "just work" in async methods, though?

@benjamn
Copy link
Member Author

benjamn commented Jul 29, 2016

@shicks That's right, though I would never propose a synchronous HTTP request. Instead, the idea was that there must be a deadline for the asynchronous part of module fetching, and that deadline would come before evaluation of the original entry point module (potentially long before any nested import declarations are evaluated). The TC39 committee objected specifically to that long-distance separation of (unconditional) data fetching from (conditional) module evaluation, and I agree with their concerns.

The essence of the follow-up proposal is overturning the requirement of synchronous evaluation in favor of making all import declarations asynchronous. This makes the proposal compatible with top-level await (which the current proposal would make very difficult), and allows import declarations anywhere an await expression statement could appear, which covers virtually all of my original use cases. To reflect the newly-visible asynchronous behavior of imports, it's likely that there will be a new syntax for nested imports, e.g. await import … instead of import ….

An alternative approach for asynchronous imports would be an imperative API that takes a module identifier and returns a Promise<exports>. This is an idea that was discussed at the previous TC39 meeting, and suggested by @dherman, I believe. However, abandoning the syntax of ES2015 import declarations would sacrifice things like live binding and easy static analysis of imported symbols. The imperative API is definitely still on the table, especially for cases when the source module identifier is only known at runtime, but we think a syntax like await import { a, b as c } from "./module" would retain the static analysis characteristics of ES2015 import declarations as we know them.

Credit to @dherman, @caridy, @wycats, @samth, and others for their input into designing this static asynchronous syntax. More details to come (in a separate proposal)!

@shicks
Copy link

shicks commented Jul 29, 2016

Very cool! The await import syntax seems so obvious now that you mention it. It would be great to see a dynamic option also available, but hopefully that won't block moving forward with this.

Is the idea with top-level await that you could resolve a circular dependency explicitly by writing both as top-level imports but adding await to the hard dependency? That would allow writing @concavelenz's example with top-level imports and no async methods, while still explicitly stating the dependency order for any other tooling.

@hemanth
Copy link
Member

hemanth commented Jul 31, 2016

Do you have some quick sample code for https://git.io/es-next? @benjamn

Using:

import { check as checkClient } from "./client.js";
import { check as checkServer } from "./server.js";
import { check as checkBoth } from "./both.js";

For now.

@Jessidhia
Copy link

@hemanth that is the code this proposal wants to replace and is presented as bad code.

The correct way is the first example in https://github.com/benjamn/reify/blob/master/PROPOSAL.md#isolated-scopes

@hemanth
Copy link
Member

hemanth commented Aug 1, 2016

@Kovensky It makes sense, isolated scopes, rings the bell.

describe("fancy feature #5", () => {
  import { strictEqual } from "assert";

  it("should work on the client", () => {
    import { check } from "./client.js";
    strictEqual(check(), "client ok");
  });

  it("should work on the client", () => {
    import { check } from "./server.js";
    strictEqual(check(), "server ok");
  });

  it("should work on both client and server", () => {
    import { check } from "./both.js";
    strictEqual(check(), "both ok");
  });
});

@concavelenz
Copy link

I would have to see a specific "async" import proposal to understand if it
solves the circular imports issues. If any downstream imports are blocked
on the "async import" then it doesn't solve the circular imports. The
scope'd "fetched but un-evaluated" imports proposed here (and I understand
it) do: the top-level declared imports are "hard" and must be fetched and,
in the absence of "hard" circular dependencies, eval'd but the lazy local
scope'd deps are "soft".

As the class hierarchy requires "hard" dependencies (the extend expression
is evaluated at the time the class declaration is initialized), using
"soft" dependencies for imports referenced in methods make it possible to
break circular references.

On Fri, Jul 29, 2016 at 2:10 PM, Stephen Hicks [email protected]
wrote:

Very cool! The await import syntax seems so obvious now that you mention
it. It would be great to see a dynamic option also available, but hopefully
that won't block moving forward with this.

Is the idea with top-level await that you could resolve a circular
dependency explicitly by writing both as top-level imports but adding
await to the hard dependency? That would allow writing @concavelenz
https://github.com/concavelenz's example with top-level imports and no
async methods, while still explicitly stating the dependency order for any
other tooling.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#646 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABMDKhgbZcBOV1A0JChLVZCvYO0Jxvdpks5qamxRgaJpZM4JVcli
.

@bterlson bterlson added the needs consensus This needs committee consensus before it can be eligible to be merged. label Aug 9, 2016
@bterlson
Copy link
Member

bterlson commented Sep 29, 2016

@benjamn Closing this as we're not anywhere close to accepting this PR.

@bterlson bterlson closed this Sep 29, 2016
@ljharb
Copy link
Member

ljharb commented Sep 29, 2016

(wrong @benjamn, i think)

@bterlson
Copy link
Member

@ljharb yes thanks

@benderTheCrime
Copy link

I opened an issue here: tc39/proposals#70 to get some clarity around the intent of this proposal and what the differences are in between this and the functional import() proposal, but I'm having difficulty following this PR thread. Is there any update on whether this will be made into its own repo with a README?

@Jessidhia
Copy link

Jessidhia commented Aug 29, 2017

From what I could gather, the purpose of this is to restrict the scope of imported names and make it easier for dead code elimination to remove imports. The import still executes synchronously but at the top of the enclosing block.

The dynamic import() always runs asynchronously and is incredibly hard to analyze for DCE. However, it seems to be the opinion of the committee that no imports are synchronous at all (top level imports just happen to wait for the dependency graph to execute before the importing module starts executing), so it is not feasible to add any other synchronous means to import.

EDIT: this makes it sound like a top-level await, but it is not the same. A top-level await can cause a wait at any point in the body, but the hoisted imports always "await" and finish executing before the body starts executing at all, barring cycles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants