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

improve dynamic reexporting #5452

Merged
merged 7 commits into from
Jul 4, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/turbopack-ecmascript-runtime/js/src/build/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ interface TurbopackNodeBuildContext {
f: RequireContextFactory;
i: EsmImport;
s: EsmExport;
j: typeof cjsExport;
j: typeof dynamicExport;
v: ExportValue;
n: typeof exportNamespace;
m: Module;
Expand Down Expand Up @@ -182,7 +182,7 @@ function instantiateModule(id: ModuleId, source: SourceInfo): Module {
f: requireContext.bind(null, module),
i: esmImport.bind(null, module),
s: esm.bind(null, module.exports),
j: cjsExport.bind(null, module.exports),
j: dynamicExport.bind(null, module),
v: exportValue.bind(null, module),
n: exportNamespace.bind(null, module),
m: module,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ interface TurbopackDevBaseContext {
f: RequireContextFactory;
i: EsmImport;
s: EsmExport;
j: typeof cjsExport;
j: typeof dynamicExport;
v: ExportValue;
n: typeof exportNamespace;
m: Module;
Expand Down Expand Up @@ -337,7 +337,7 @@ function instantiateModule(id: ModuleId, source: SourceInfo): Module {
f: requireContext.bind(null, module),
i: esmImport.bind(null, module),
s: esmExport.bind(null, module),
j: cjsExport.bind(null, module.exports),
j: dynamicExport.bind(null, module),
v: exportValue.bind(null, module),
n: exportNamespace.bind(null, module),
m: module,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ interface Exports {
}
type EsmNamespaceObject = Record<string, any>;

const REEXPORTED_OBJECTS = Symbol("reexported objects");

interface BaseModule {
exports: Exports;
error: Error | undefined;
Expand All @@ -24,6 +26,7 @@ interface BaseModule {
children: ModuleId[];
parents: ModuleId[];
namespaceObject?: EsmNamespaceObject;
[REEXPORTED_OBJECTS]?: any[];
}

interface Module extends BaseModule {}
Expand Down Expand Up @@ -81,12 +84,38 @@ function esmExport(module: Module, getters: Record<string, () => any>) {
}

/**
* Adds the props to the exports object
* Dynamically exports properties from an object
*/
function cjsExport(exports: Exports, props: Record<string, any>) {
for (const key in props) {
defineProp(exports, key, { get: () => props[key], enumerable: true });
function dynamicExport(module: Module, object: Record<string, any>) {
if (!module[REEXPORTED_OBJECTS]) {
module[REEXPORTED_OBJECTS] = [];
sokra marked this conversation as resolved.
Show resolved Hide resolved
module.namespaceObject = new Proxy(module.exports, {
get(target, prop) {
if (
hasOwnProperty.call(target, prop) ||
prop === "default" ||
prop === "__esModule"
) {
return Reflect.get(target, prop);
}
for (const obj of module[REEXPORTED_OBJECTS]!) {
sokra marked this conversation as resolved.
Show resolved Hide resolved
const value = Reflect.get(obj, prop);
if (value !== undefined) return value;
}
return undefined;
},
ownKeys(target) {
const keys = Reflect.ownKeys(target);
for (const obj of module[REEXPORTED_OBJECTS]!) {
sokra marked this conversation as resolved.
Show resolved Hide resolved
for (const key of Reflect.ownKeys(obj)) {
if (key !== "default" && !keys.includes(key)) keys.push(key);
}
}
return keys;
},
});
}
module[REEXPORTED_OBJECTS]!.push(object);
sokra marked this conversation as resolved.
Show resolved Hide resolved
}

function exportValue(module: Module, value: any) {
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-ecmascript/src/chunk/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ impl EcmascriptChunkItemContentVc {
"n: __turbopack_export_namespace__",
"c: __turbopack_cache__",
"l: __turbopack_load__",
"j: __turbopack_cjs__",
"j: __turbopack_dynamic__",
"g: global",
// HACK
"__dirname",
Expand Down
1 change: 1 addition & 0 deletions crates/turbopack-ecmascript/src/chunk/placeable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ impl EcmascriptChunkPlaceablesVc {
#[turbo_tasks::value(shared)]
pub enum EcmascriptExports {
EsmExports(EsmExportsVc),
DynamicNamespace,
CommonJs,
Value,
None,
Expand Down
27 changes: 15 additions & 12 deletions crates/turbopack-ecmascript/src/references/esm/export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ pub enum EsmExport {
#[turbo_tasks::value]
struct ExpandResults {
star_exports: Vec<String>,
has_cjs_exports: bool,
has_dynamic_exports: bool,
}

#[turbo_tasks::function]
async fn expand_star_exports(root_asset: EcmascriptChunkPlaceableVc) -> Result<ExpandResultsVc> {
let mut set = HashSet::new();
let mut has_cjs_exports = false;
let mut has_dynamic_exports = false;
let mut checked_assets = HashSet::new();
checked_assets.insert(root_asset);
let mut queue = vec![(root_asset, root_asset.get_exports())];
Expand Down Expand Up @@ -100,7 +100,7 @@ async fn expand_star_exports(root_asset: EcmascriptChunkPlaceableVc) -> Result<E
.as_issue()
.emit(),
EcmascriptExports::CommonJs => {
has_cjs_exports = true;
has_dynamic_exports = true;
AnalyzeIssue {
code: None,
category: StringVc::cell("analyze".to_string()),
Expand All @@ -120,11 +120,14 @@ async fn expand_star_exports(root_asset: EcmascriptChunkPlaceableVc) -> Result<E
.as_issue()
.emit()
}
EcmascriptExports::DynamicNamespace => {
has_dynamic_exports = true;
}
}
}
Ok(ExpandResultsVc::cell(ExpandResults {
star_exports: set.into_iter().collect(),
has_cjs_exports,
has_dynamic_exports,
}))
}

Expand All @@ -151,7 +154,7 @@ impl CodeGenerateable for EsmExports {
.map(|(k, v)| (Cow::<str>::Borrowed(k), Cow::Borrowed(v)))
.collect();
let mut props = Vec::new();
let mut cjs_exports = Vec::<Box<Expr>>::new();
let mut dynamic_exports = Vec::<Box<Expr>>::new();

for esm_ref in this.star_exports.iter() {
if let ReferencedAsset::Some(asset) = &*esm_ref.get_referenced_asset().await? {
Expand All @@ -166,11 +169,11 @@ impl CodeGenerateable for EsmExports {
}
}

if export_info.has_cjs_exports {
if export_info.has_dynamic_exports {
let ident = ReferencedAsset::get_ident_from_placeable(asset).await?;

cjs_exports.push(quote_expr!(
"__turbopack_cjs__($arg)",
dynamic_exports.push(quote_expr!(
"__turbopack_dynamic__($arg)",
arg: Expr = Ident::new(ident.into(), DUMMY_SP).into()
));
}
Expand Down Expand Up @@ -230,10 +233,10 @@ impl CodeGenerateable for EsmExports {
span: DUMMY_SP,
props,
});
let cjs_stmt = if !cjs_exports.is_empty() {
let dynamic_stmt = if !dynamic_exports.is_empty() {
Some(Stmt::Expr(ExprStmt {
span: DUMMY_SP,
expr: Expr::from_exprs(cjs_exports),
expr: Expr::from_exprs(dynamic_exports),
}))
} else {
None
Expand All @@ -251,8 +254,8 @@ impl CodeGenerateable for EsmExports {
body.insert(0, stmt);
}
}
if let Some(cjs_stmt) = cjs_stmt.clone() {
insert_hoisted_stmt(program, cjs_stmt);
if let Some(dynamic_stmt) = dynamic_stmt.clone() {
insert_hoisted_stmt(program, dynamic_stmt);
}
}));

Expand Down
108 changes: 80 additions & 28 deletions crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -588,27 +588,48 @@ pub(crate) async fn analyze_ecmascript_module(

EcmascriptExports::EsmExports(esm_exports)
} else if matches!(specified_type, SpecifiedModuleType::EcmaScript) {
if has_cjs_export(program) {
SpecifiedModuleTypeIssue {
path: source.ident().path(),
specified_type,
match detect_dynamic_export(program) {
DetectedDynamicExportType::CommonJs => {
SpecifiedModuleTypeIssue {
path: source.ident().path(),
specified_type,
}
.cell()
.as_issue()
.emit();
EcmascriptExports::EsmExports(
EsmExports {
exports: Default::default(),
star_exports: Default::default(),
}
.cell(),
)
}
.cell()
.as_issue()
.emit();
DetectedDynamicExportType::Namespace => EcmascriptExports::DynamicNamespace,
DetectedDynamicExportType::Value => EcmascriptExports::Value,
Comment on lines +608 to +609
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think this unfortunately breaks my async module implementation 🙃

DetectedDynamicExportType::UsingModuleDeclarations
| DetectedDynamicExportType::None => EcmascriptExports::EsmExports(
EsmExports {
exports: Default::default(),
star_exports: Default::default(),
}
.cell(),
),
}

EcmascriptExports::EsmExports(
EsmExports {
exports: Default::default(),
star_exports: Default::default(),
}
.cell(),
)
} else if has_cjs_export(program) {
EcmascriptExports::CommonJs
} else {
EcmascriptExports::None
match detect_dynamic_export(program) {
DetectedDynamicExportType::CommonJs => EcmascriptExports::CommonJs,
DetectedDynamicExportType::Namespace => EcmascriptExports::DynamicNamespace,
DetectedDynamicExportType::Value => EcmascriptExports::Value,
DetectedDynamicExportType::UsingModuleDeclarations => EcmascriptExports::EsmExports(
EsmExports {
exports: Default::default(),
star_exports: Default::default(),
}
.cell(),
),
DetectedDynamicExportType::None => EcmascriptExports::None,
}
};

analysis.set_exports(exports);
Expand Down Expand Up @@ -2553,18 +2574,27 @@ pub struct AstPath(#[turbo_tasks(trace_ignore)] Vec<AstParentKind>);
pub static TURBOPACK_HELPER: &str = "__turbopackHelper";

pub fn is_turbopack_helper_import(import: &ImportDecl) -> bool {
import.asserts.as_ref().map_or(true, |asserts| {
import.asserts.as_ref().map_or(false, |asserts| {
asserts.props.iter().any(|assert| {
assert
.as_prop()
.and_then(|prop| prop.as_key_value())
.and_then(|kv| kv.key.as_ident())
.map_or(true, |ident| &*ident.sym != TURBOPACK_HELPER)
.map_or(false, |ident| &*ident.sym == TURBOPACK_HELPER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this broken before, or was it used some other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was broken before

})
})
}

fn has_cjs_export(p: &Program) -> bool {
#[derive(Debug)]
enum DetectedDynamicExportType {
CommonJs,
Namespace,
Value,
None,
UsingModuleDeclarations,
}

fn detect_dynamic_export(p: &Program) -> DetectedDynamicExportType {
use swc_core::ecma::visit::{visit_obj_and_computed, Visit, VisitWith};

if let Program::Module(m) = p {
Expand All @@ -2576,22 +2606,31 @@ fn has_cjs_export(p: &Program) -> bool {
.map_or(true, |import| !is_turbopack_helper_import(import))
})
}) {
return false;
return DetectedDynamicExportType::UsingModuleDeclarations;
}
}

struct Visitor {
cjs: bool,
value: bool,
namespace: bool,
found: bool,
}

impl Visit for Visitor {
visit_obj_and_computed!();

fn visit_ident(&mut self, i: &Ident) {
if &*i.sym == "module"
|| &*i.sym == "exports"
|| &*i.sym == "__turbopack_export_value__"
{
if &*i.sym == "module" || &*i.sym == "exports" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this only work if the ident is unresolved? e.g. will it false positive const module = {}?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's broken in that case. It's only a difference between no exports and commonjs exports, and doesn't matter at runtime. It generates a bit less code for "no exports", but nevermind...

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining this, so we know that this is broken but it doesn't actually matter?

self.cjs = true;
self.found = true;
}
if &*i.sym == "__turbopack_export_value__" {
self.value = true;
self.found = true;
}
if &*i.sym == "__turbopack_export_namespace__" {
self.namespace = true;
self.found = true;
}
}
Expand All @@ -2610,7 +2649,20 @@ fn has_cjs_export(p: &Program) -> bool {
}
}

let mut v = Visitor { found: false };
let mut v = Visitor {
cjs: false,
value: false,
namespace: false,
found: false,
};
p.visit_with(&mut v);
v.found
if v.cjs {
DetectedDynamicExportType::CommonJs
} else if v.value {
DetectedDynamicExportType::Value
} else if v.namespace {
DetectedDynamicExportType::Namespace
} else {
DetectedDynamicExportType::None
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading