tc39 / proposal-esm-phase-imports

https://tc39.es/proposal-esm-phase-imports/
MIT License
29 stars 1 forks source link

Module declarations analysis support #19

Open guybedford opened 1 week ago

guybedford commented 1 week ago

Module analysis will need to expand to support future module syntax features.

In particular, module declarations will likely introduce the concept of direct module linkage:

module x {
  import y;
}
module y {
  console.log('y');
}
import(x); // logs 'y'

The question for the analysis is then whether we include this import in the analysis data or not when checking x.imports().

We could have a special entry for the import like { type: 'declaration', declarationIdentifier: 'y' }.

Alternatively we could even include the direct declaration object in this analysis - { type: 'declaration', module: ModuleSource Y { ... } }.

We should ensure we have a good answer here that aligns with common future use cases.

nicolo-ribaudo commented 5 days ago

I would not expose those "direct" internal imports in the reflection API, since they are all bindings-based and not something that the host can control. It is an internal implementation detail of the module. And even if we expose them, I would absolutely not expose the name.

What we need to figure out is how to represent the external imports inside a module declaration:

module x {
  import "a";
}
import "b";

Does this module have "a" and "b" as imports? "b" and "a"? Does it have only "b"?

nicolo-ribaudo commented 5 days ago

One option is to pre-resolve external imports in local imported module declaration, and provide more data for exported declarations. For example:

import { x } from "./a";
module y {
  import "./b";
}
export module z {
  import "./c";
}

import y;
import "./d";
import x;

could be:

mod.imports() == [
  { "specifier": "./a" },
  { "specifier": "./b" },
  { "specifier": "./d" },
  { "specifier": "./a", "module": "x" }
];
mod.namedExports() == ["z"];
mod.wildcardExports() == [];

const modZ = mod.getExportedModule("z");
modZ.imports() == [
  { "specifier": "./c" }
]

This exposes info about the exported/imported modules, since they affect linkage of files, while keeping internal-only modules that the host will never actually see or interact with as an internal unexposed details.

guybedford commented 5 days ago

I would say that until the module is explicitly imported, the dependency of a module declaration is not a dependency of a module itself.

That is, it is possible to have:

import 'z';
module x {
  import 'this-is-never-ever-imported';
}

where because there is no import(x) and the module x is not exposed externally, the import is never imported.

Therefore, if anything, I think a new analysis property may be useful to interrogate module expressions within a module source:

module.imports() == [{ specifier: 'z' }]
module.declarations() = [ModuleSource x {
  imports() // = [{ specifier: 'this-is-never-ever-imported' }]
}]

Where arbitrary nesting can also be inferred by recursively checking for declarations() on the declarations themselves.

ljharb commented 5 days ago

Why would a non exported/imported module declaration - this, a completely unused one - need analysis at all?

guybedford commented 5 days ago

Yeah without knowledge of the name and linkage that information may be unnecessary, but one can imagine wanting to analyze bundles to know what modules they contain.

ljharb commented 5 days ago

It shouldn’t be observable that I’ve added unused code, just like it’s not observable if i add an unused function declaration.

guybedford commented 5 days ago

Right, perhaps we only want exported declarations to be observable then.

It's just hard to trace if there's indirection:

module mod {
}
const x = mod;
export { x };
ljharb commented 5 days ago

exported, or imported, presumably. also i'm not sure why that's hard to trace within a single file?

guybedford commented 5 days ago

Actually, the ability to import a module declaration only works across static bindings anyway, so the above won't work for module declarations:

// mod.js
module mod {
}
const x = mod;
export { x }

// main.js
import { x } from './mod.js';
import x; // THROWS "ReferenceError: Module declaration not statically defined" (or something like that)
import(x); // WORKS

That is, statically importable module declarations must be static bindings.

Perhaps it's okay then that only statically exposed (since exposed is now a well-defined property) module declarations are possible to gather analysis for?

guybedford commented 5 days ago

All this to say, perhaps the API is even on exports itself (in the static exported declaration case)?

module.exports() = [{ exportName: 'x', declaration: ModuleSource mod { } }]
guybedford commented 5 days ago

To complete the discussion here, considering the analysis of:

module x {
}
import { yMod as z } from 'y';

import x;
import z;

I agree with @nicolo-ribaudo that the name should be an internal detail, but the yMod name is useful to know in cross-bundle analysis per the discussion about exposed declarations above.

So in the above, maybe we can say that the module x {} is fully private from a public API perspective so that only the import z is something about which there is analysis available?

I'm then wondering if we might still extend imports analysis to provide a separate declarationImports() analysis, perhaps along the lines of:

mod.declarationImports() = [{ importName: 'yMod', specifier: 'y' }]