From 57a231a7703a87358980cf826fcc799d6dc11e9d Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Wed, 10 Apr 2024 14:01:02 +1000 Subject: [PATCH 01/14] WIP --- .../bundlers/default/src/DefaultBundler.js | 5 +- packages/core/core/src/BundleGraph.js | 27 +++++- packages/core/core/src/public/BundleGraph.js | 20 ++++ .../core/src/public/MutableBundleGraph.js | 19 ++++ packages/core/core/src/types.js | 2 + packages/core/types-internal/src/index.js | 5 +- .../packagers/js/src/ScopeHoistingPackager.js | 13 +++ packages/packagers/js/src/utils.js | 2 +- packages/runtimes/js/src/JSRuntime.js | 4 + .../js/src/helpers/bundle-manifest.js | 9 +- .../js/core/src/dependency_collector.rs | 94 +++++++++++++++---- packages/transformers/js/core/src/lib.rs | 2 + packages/transformers/js/src/JSTransformer.js | 10 +- 13 files changed, 186 insertions(+), 26 deletions(-) diff --git a/packages/bundlers/default/src/DefaultBundler.js b/packages/bundlers/default/src/DefaultBundler.js index 23a7282028e..04086a48ba3 100644 --- a/packages/bundlers/default/src/DefaultBundler.js +++ b/packages/bundlers/default/src/DefaultBundler.js @@ -95,6 +95,7 @@ const dependencyPriorityEdges = { sync: 1, parallel: 2, lazy: 3, + conditional: 4, }; type DependencyBundleGraph = ContentGraph< @@ -495,7 +496,8 @@ function createIdealGraph( if ( node.type === 'dependency' && - node.value.priority === 'lazy' && + (node.value.priority === 'lazy' || + node.value.priority === 'conditional') && parentAsset ) { // Don't walk past the bundle group assets @@ -584,6 +586,7 @@ function createIdealGraph( } if ( dependency.priority === 'lazy' || + dependency.priority === 'conditional' || childAsset.bundleBehavior === 'isolated' // An isolated Dependency, or Bundle must contain all assets it needs to load. ) { if (bundleId == null) { diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index a4a9122056c..03ccfb34b15 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -87,6 +87,7 @@ type BundleGraphOpts = {| bundleContentHashes: Map, assetPublicIds: Set, publicIdByAssetId: Map, + conditions: Map, |}; type SerializedBundleGraph = {| @@ -95,6 +96,7 @@ type SerializedBundleGraph = {| bundleContentHashes: Map, assetPublicIds: Set, publicIdByAssetId: Map, + conditions: Map, |}; function makeReadOnlySet(set: Set): $ReadOnlySet { @@ -135,22 +137,26 @@ export default class BundleGraph { /** The internal core Graph structure */ _graph: ContentGraph; _bundlePublicIds /*: Set */ = new Set(); + _conditions /*: Map */ = new Map(); constructor({ graph, publicIdByAssetId, assetPublicIds, bundleContentHashes, + conditions, }: {| graph: ContentGraph, publicIdByAssetId: Map, assetPublicIds: Set, bundleContentHashes: Map, + conditions: Map, |}) { this._graph = graph; this._assetPublicIds = assetPublicIds; this._publicIdByAssetId = publicIdByAssetId; this._bundleContentHashes = bundleContentHashes; + this._conditions = conditions; } /** @@ -167,6 +173,7 @@ export default class BundleGraph { let assetGroupIds = new Map(); let dependencies = new Map(); let assetGraphNodeIdToBundleGraphNodeId = new Map(); + let conditions = new Map(); let assetGraphRootNode = assetGraph.rootNodeId != null @@ -198,6 +205,20 @@ export default class BundleGraph { walkVisited.add(nodeId); let node = nullthrows(assetGraph.getNode(nodeId)); + + if (node.type === 'asset') { + const asset = node.value; + if (Array.isArray(asset.meta.conditions)) { + for (const _condition of asset.meta.conditions ?? []) { + const condition = String(_condition); + const condHash = hashString(condition); + const condPublicId = getPublicId(condHash, v => conditions.has(v)); + // FIXME is this the right way around?? It is for packaging.. + conditions.set(condition, condPublicId); + } + } + } + if ( node.type === 'dependency' && node.value.symbols != null && @@ -439,6 +460,7 @@ export default class BundleGraph { assetPublicIds, bundleContentHashes: new Map(), publicIdByAssetId, + conditions, }); } @@ -449,6 +471,7 @@ export default class BundleGraph { assetPublicIds: this._assetPublicIds, bundleContentHashes: this._bundleContentHashes, publicIdByAssetId: this._publicIdByAssetId, + conditions: this._conditions, }; } @@ -458,6 +481,7 @@ export default class BundleGraph { assetPublicIds: serialized.assetPublicIds, bundleContentHashes: serialized.bundleContentHashes, publicIdByAssetId: serialized.publicIdByAssetId, + conditions: serialized.conditions, }); } @@ -1209,7 +1233,8 @@ export default class BundleGraph { .some( node => node?.type === 'dependency' && - node.value.priority === Priority.lazy && + (node.value.priority === Priority.lazy || + node.value.priority === Priority.conditional) && node.value.specifierType !== SpecifierType.url, ) ) { diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index 39d12920d42..f06fc538de1 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -326,4 +326,24 @@ export default class BundleGraph targetToInternalTarget(target), ); } + + getConditionPublicId(condition: string): string { + if (!this.#graph._conditions.has(condition)) { + throw new Error(`Condition ${condition} was not in mapping`); + } + return this.#graph._conditions.get(condition) ?? ''; + } + + getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { + const ret = {}; + for (const [k, v] of this.#graph._conditions.entries()) { + const [feature, ifTruePlaceholder, ifFalsePlaceholder] = k.split(':'); + ret[v] = { + ff: feature, + t: ifTruePlaceholder, + f: ifFalsePlaceholder, + }; + } + return ret; + } } diff --git a/packages/core/core/src/public/MutableBundleGraph.js b/packages/core/core/src/public/MutableBundleGraph.js index 7144aad779c..474d379eea0 100644 --- a/packages/core/core/src/public/MutableBundleGraph.js +++ b/packages/core/core/src/public/MutableBundleGraph.js @@ -38,6 +38,7 @@ export default class MutableBundleGraph #graph /*: InternalBundleGraph */; #options /*: ParcelOptions */; #bundlePublicIds /*: Set */ = new Set(); + #bundleConditions /* : Map */ = new Map(); constructor(graph: InternalBundleGraph, options: ParcelOptions) { super(graph, Bundle.get.bind(Bundle), options); @@ -50,6 +51,20 @@ export default class MutableBundleGraph assetToAssetValue(asset), bundleToInternalBundle(bundle), ); + + if (asset.meta.conditions != null && Array.isArray(asset.meta.conditions)) { + for (const _condition of asset.meta.conditions ?? []) { + const condition = String(_condition); + if (!this.#bundleConditions.has(condition)) { + const condHash = hashString(condition); + const condPublicId = getPublicId(condHash, v => + this.#bundleConditions.has(v), + ); + // FIXME is this the right way around?? It is for packaging.. + this.#bundleConditions.set(condition, condPublicId); + } + } + } } addAssetGraphToBundle( @@ -317,4 +332,8 @@ export default class MutableBundleGraph bundleToInternalBundle(bundle), ); } + + getConditions(): Map { + return this.#bundleConditions; + } } diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js index b355f968b09..59353d85b9e 100644 --- a/packages/core/core/src/types.js +++ b/packages/core/core/src/types.js @@ -114,6 +114,7 @@ export const Priority = { sync: 0, parallel: 1, lazy: 2, + conditional: 3, }; // Must match package_json.rs in node-resolver-rs. @@ -540,6 +541,7 @@ export type Bundle = {| displayName: ?string, pipeline: ?string, manualSharedBundle?: ?string, + conditions?: Map, |}; export type BundleNode = {| diff --git a/packages/core/types-internal/src/index.js b/packages/core/types-internal/src/index.js index 39b4e9bf9f1..6a4299a1bcd 100644 --- a/packages/core/types-internal/src/index.js +++ b/packages/core/types-internal/src/index.js @@ -529,7 +529,7 @@ export interface MutableDependencySymbols // eslint-disable-next-line no-undef delete(exportSymbol: Symbol): void; } -export type DependencyPriority = 'sync' | 'parallel' | 'lazy'; +export type DependencyPriority = 'sync' | 'parallel' | 'lazy' | 'conditional'; export type SpecifierType = 'commonjs' | 'esm' | 'url' | 'custom'; /** @@ -1294,6 +1294,7 @@ export type CreateBundleOpts = +bundleBehavior?: ?BundleBehavior, /** Name of the manual shared bundle config that caused this bundle to be created */ +manualSharedBundle?: ?string, + +conditions?: Array, |} // If an entryAsset is not provided, a bundle id, type, and environment must // be provided. @@ -1329,6 +1330,7 @@ export type CreateBundleOpts = +pipeline?: ?string, /** Name of the manual shared bundle config that caused this bundle to be created */ +manualSharedBundle?: ?string, + +conditions?: Array, |}; /** @@ -1611,6 +1613,7 @@ export interface BundleGraph { getUsedSymbols(Asset | Dependency): ?$ReadOnlySet; /** Returns the common root directory for the entry assets of a target. */ getEntryRoot(target: Target): FilePath; + getConditionPublicId(condition: string): string; } /** diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index ed58dcb7f91..ae7c2786356 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -110,6 +110,8 @@ export class ScopeHoistingPackager { this.parcelRequireName = parcelRequireName; this.useAsyncBundleRuntime = useAsyncBundleRuntime; + // console.log("CONDTIONS", bundleGraph.getConditions()); + let OutputFormat = OUTPUT_FORMATS[this.bundle.env.outputFormat]; this.outputFormat = new OutputFormat(this); @@ -614,6 +616,17 @@ export class ScopeHoistingPackager { } return replacement; }); + + // Handle conditional imports + if (code.includes('__parcel__requireCond__')) { + console.log("Let's handle conditional imports!"); + const IMPORT_COND_RE = /__parcel__requireCond__\(['"](.*?)['"]\)/g; + code = code.replace(IMPORT_COND_RE, (m, s) => { + console.log(`_P_R_C_`, m, s); + const condId = this.bundleGraph.getConditionPublicId(s); + return `parcelRequire("cond:${condId}")`; + }); + } } // If the asset is wrapped, we need to insert the dependency code outside the parcelRequire.register diff --git a/packages/packagers/js/src/utils.js b/packages/packagers/js/src/utils.js index 59e981f43eb..b9cb88c41af 100644 --- a/packages/packagers/js/src/utils.js +++ b/packages/packagers/js/src/utils.js @@ -25,7 +25,7 @@ export function replaceScriptDependencies( lineCount++; return '\n'; } - + console.log(`_P_R_`, s); let dep = nullthrows(dependencies.find(d => getSpecifier(d) === s)); let resolved = nullthrows(bundleGraph.getResolvedAsset(dep, bundle)); let publicId = bundleGraph.getAssetPublicId(resolved); diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index d66e1d6b530..c6e6499f64b 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -660,8 +660,12 @@ function getRegisterCode( ? 'new __parcel__URL__("").toString()' // <-- this isn't ideal. We should use `import.meta.url` directly but it gets replaced currently : `require('./helpers/bundle-url').getBundleURL('${entryBundle.publicId}')`; + const conditions = bundleGraph.getConditionMapping(); + return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( JSON.stringify(mappings), + )}));require('./helpers/bundle-manifest').registerConditions(JSON.parse(${JSON.stringify( + JSON.stringify(conditions), )}));`; } diff --git a/packages/runtimes/js/src/helpers/bundle-manifest.js b/packages/runtimes/js/src/helpers/bundle-manifest.js index 916b789e772..4b28d19bf4e 100644 --- a/packages/runtimes/js/src/helpers/bundle-manifest.js +++ b/packages/runtimes/js/src/helpers/bundle-manifest.js @@ -1,5 +1,5 @@ var mapping = new Map(); - +var conditions = new Map(); function register(baseUrl, manifest) { for (var i = 0; i < manifest.length - 1; i += 2) { mapping.set(manifest[i], { @@ -17,5 +17,12 @@ function resolve(id) { return new URL(resolved.path, resolved.baseUrl).toString(); } +function registerConditions(conditions) { + for (const [k, v] of Object.entries(conditions)) { + conditions.set(k, v); + } +} + module.exports.register = register; module.exports.resolve = resolve; +module.exports.registerConditions = registerConditions; diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index 329c09355d0..6c93909ff2a 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -1,10 +1,10 @@ use path_slash::PathBufExt; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; use std::path::Path; use serde::{Deserialize, Serialize}; -use swc_core::common::{Mark, SourceMap, Span, DUMMY_SP}; +use swc_core::common::{Mark, SourceMap, Span, Spanned, DUMMY_SP}; use swc_core::ecma::ast::{self, Callee, MemberProp}; use swc_core::ecma::atoms::{js_word, JsWord}; use swc_core::ecma::visit::{Fold, FoldWith}; @@ -28,6 +28,7 @@ pub enum DependencyKind { Import, Export, DynamicImport, + ConditionalImport, Require, WebWorker, ServiceWorker, @@ -63,6 +64,7 @@ pub fn dependency_collector<'a>( unresolved_mark: swc_core::common::Mark, config: &'a Config, diagnostics: &'a mut Vec, + conditions: &'a mut HashSet, ) -> impl Fold + 'a { DependencyCollector { source_map, @@ -75,6 +77,7 @@ pub fn dependency_collector<'a>( config, diagnostics, import_meta: None, + conditions, } } @@ -89,6 +92,7 @@ struct DependencyCollector<'a> { config: &'a Config, diagnostics: &'a mut Vec, import_meta: Option, + conditions: &'a mut HashSet, } impl<'a> DependencyCollector<'a> { @@ -454,6 +458,7 @@ impl<'a> Fold for DependencyCollector<'a> { )))); return call; } + "importCond" => DependencyKind::ConditionalImport, _ => return node.fold_children_with(self), } } @@ -644,26 +649,29 @@ impl<'a> Fold for DependencyCollector<'a> { return node; } - let placeholder = self.add_dependency( - specifier, - span, - kind.clone(), - attributes, - kind == DependencyKind::Require && self.in_try, - self.config.source_type, - ); - - if let Some(placeholder) = placeholder { - let mut node = node.clone(); - node.args[0].expr = Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { - value: placeholder, - span, - raw: None, - }))); - node + if kind == DependencyKind::ConditionalImport { + println!("Conditional import for feature {}", specifier); } else { - node + let placeholder = self.add_dependency( + specifier, + span, + kind.clone(), + attributes, + kind == DependencyKind::Require && self.in_try, + self.config.source_type, + ); + + if let Some(placeholder) = placeholder { + let mut node = node.clone(); + node.args[0].expr = Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { + value: placeholder, + span, + raw: None, + }))); + return node; + } } + node } else { node } @@ -695,6 +703,52 @@ impl<'a> Fold for DependencyCollector<'a> { } else if kind == DependencyKind::Require { // Don't continue traversing so that the `require` isn't replaced with undefined rewrite_require_specifier(node, self.unresolved_mark) + } else if kind == DependencyKind::ConditionalImport { + let mut call = node; + call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( + "__parcel__requireCond__".into(), + DUMMY_SP, + )))); + + if call.args.len() != 3 { + // FIXME make this a diagnostic + panic!("importCond requires 3 arguments"); + } + let mut placeholders = Vec::new(); + for arg in &call.args[1..] { + let specifier = match_str(&arg.expr).unwrap().0; + println!("Conditional specifier: {}", specifier); + let placeholder = self.add_dependency( + specifier, + arg.span(), + DependencyKind::ConditionalImport, + None, + true, + self.config.source_type, + ); + placeholders.push(placeholder.unwrap()); + } + + let condition: JsWord = format!( + "{}:{}:{}", + match_str(&call.args[0].expr).unwrap().0, + placeholders[0], + placeholders[1] + ) + .into(); + self.conditions.insert(condition.clone()); + + call.args[0] = ast::ExprOrSpread { + spread: None, + expr: Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { + value: condition, + span: DUMMY_SP, + raw: None, + }))), + }; + call.args.truncate(1); + + call } else { node.fold_children_with(self) } diff --git a/packages/transformers/js/core/src/lib.rs b/packages/transformers/js/core/src/lib.rs index 83814a998d2..7193205dff9 100644 --- a/packages/transformers/js/core/src/lib.rs +++ b/packages/transformers/js/core/src/lib.rs @@ -109,6 +109,7 @@ pub struct TransformResult { used_env: HashSet, has_node_replacements: bool, is_constant_module: bool, + conditions: HashSet, } fn targets_to_versions(targets: &Option>) -> Option { @@ -434,6 +435,7 @@ pub fn transform( unresolved_mark, &config, &mut diagnostics, + &mut result.conditions, ), ); diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index 5db4ac97947..2f1173a1627 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -419,6 +419,7 @@ export default (new Transformer({ used_env, has_node_replacements, is_constant_module, + conditions, } = await (transformAsync || transform)({ filename: asset.filePath, code, @@ -576,6 +577,8 @@ export default (new Transformer({ : null, }); + asset.meta.conditions = conditions; + if (is_constant_module) { asset.meta.isConstantModule = true; } @@ -850,7 +853,12 @@ export default (new Transformer({ specifier: dep.specifier, specifierType: dep.kind === 'Require' ? 'commonjs' : 'esm', loc: convertLoc(dep.loc), - priority: dep.kind === 'DynamicImport' ? 'lazy' : 'sync', + priority: + dep.kind === 'DynamicImport' + ? 'lazy' + : dep.kind === 'ConditionalImport' + ? 'conditional' + : 'sync', isOptional: dep.is_optional, meta, resolveFrom: isHelper ? __filename : undefined, From cbe24e0efc08b6df023a3c24a6c8fd116a8e5593 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Thu, 11 Apr 2024 15:02:29 +1000 Subject: [PATCH 02/14] Add feature flag to conditional bundling --- packages/bundlers/default/package.json | 1 + .../bundlers/default/src/DefaultBundler.js | 7 ++- packages/core/core/src/BundleGraph.js | 36 ++++++++++--- packages/core/core/src/public/BundleGraph.js | 36 ++++++++++--- .../core/src/public/MutableBundleGraph.js | 51 +++++++++++++------ packages/core/core/src/types.js | 6 +++ packages/core/feature-flags/src/index.js | 1 + packages/core/feature-flags/src/types.js | 6 +++ packages/packagers/js/package.json | 1 + .../packagers/js/src/ScopeHoistingPackager.js | 15 ++++-- packages/runtimes/js/src/JSRuntime.js | 19 ++++--- .../js/core/src/dependency_collector.rs | 6 +-- packages/transformers/js/core/src/lib.rs | 1 + packages/transformers/js/src/JSTransformer.js | 2 + 14 files changed, 144 insertions(+), 44 deletions(-) diff --git a/packages/bundlers/default/package.json b/packages/bundlers/default/package.json index 12141e02bb1..7437a78518f 100644 --- a/packages/bundlers/default/package.json +++ b/packages/bundlers/default/package.json @@ -21,6 +21,7 @@ }, "dependencies": { "@parcel/diagnostic": "2.12.0", + "@parcel/feature-flags": "2.12.0", "@parcel/graph": "3.2.0", "@parcel/plugin": "2.12.0", "@parcel/rust": "2.12.0", diff --git a/packages/bundlers/default/src/DefaultBundler.js b/packages/bundlers/default/src/DefaultBundler.js index 04086a48ba3..81358aa91df 100644 --- a/packages/bundlers/default/src/DefaultBundler.js +++ b/packages/bundlers/default/src/DefaultBundler.js @@ -23,6 +23,7 @@ import {validateSchema, DefaultMap, globToRegex} from '@parcel/utils'; import nullthrows from 'nullthrows'; import path from 'path'; import {encodeJSONKeyComponent} from '@parcel/diagnostic'; +import {getFeatureFlag} from '@parcel/feature-flags'; type Glob = string; @@ -497,7 +498,8 @@ function createIdealGraph( if ( node.type === 'dependency' && (node.value.priority === 'lazy' || - node.value.priority === 'conditional') && + (getFeatureFlag('conditionalBundling') && + node.value.priority === 'conditional')) && parentAsset ) { // Don't walk past the bundle group assets @@ -586,7 +588,8 @@ function createIdealGraph( } if ( dependency.priority === 'lazy' || - dependency.priority === 'conditional' || + (getFeatureFlag('conditionalBundling') && + dependency.priority === 'conditional') || childAsset.bundleBehavior === 'isolated' // An isolated Dependency, or Bundle must contain all assets it needs to load. ) { if (bundleId == null) { diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 03ccfb34b15..2c58b2917a0 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -26,6 +26,7 @@ import type { Environment, InternalSourceLocation, Target, + Condition, } from './types'; import type AssetGraph from './AssetGraph'; import type {ProjectPath} from './projectPath'; @@ -42,6 +43,7 @@ import {getBundleGroupId, getPublicId} from './utils'; import {ISOLATED_ENVS} from './public/Environment'; import {fromProjectPath, fromProjectPathRelative} from './projectPath'; import {HASH_REF_PREFIX} from './constants'; +import {getFeatureFlag} from '@parcel/feature-flags'; export const bundleGraphEdgeTypes = { // A lack of an edge type indicates to follow the edge while traversing @@ -87,7 +89,7 @@ type BundleGraphOpts = {| bundleContentHashes: Map, assetPublicIds: Set, publicIdByAssetId: Map, - conditions: Map, + conditions: Map, |}; type SerializedBundleGraph = {| @@ -96,7 +98,7 @@ type SerializedBundleGraph = {| bundleContentHashes: Map, assetPublicIds: Set, publicIdByAssetId: Map, - conditions: Map, + conditions: Map, |}; function makeReadOnlySet(set: Set): $ReadOnlySet { @@ -137,7 +139,7 @@ export default class BundleGraph { /** The internal core Graph structure */ _graph: ContentGraph; _bundlePublicIds /*: Set */ = new Set(); - _conditions /*: Map */ = new Map(); + _conditions /*: Map */ = new Map(); constructor({ graph, @@ -150,7 +152,7 @@ export default class BundleGraph { publicIdByAssetId: Map, assetPublicIds: Set, bundleContentHashes: Map, - conditions: Map, + conditions: Map, |}) { this._graph = graph; this._assetPublicIds = assetPublicIds; @@ -175,6 +177,8 @@ export default class BundleGraph { let assetGraphNodeIdToBundleGraphNodeId = new Map(); let conditions = new Map(); + let placeholderToDependency = new Map(); + let assetGraphRootNode = assetGraph.rootNodeId != null ? assetGraph.getNode(assetGraph.rootNodeId) @@ -196,6 +200,15 @@ export default class BundleGraph { } } else if (node != null && node.type === 'asset_group') { assetGroupIds.set(nodeId, assetGraph.getNodeIdsConnectedFrom(nodeId)); + } else if ( + getFeatureFlag('conditionalBundling') && + node != null && + node.type === 'dependency' + ) { + const dep = node.value; + if (dep.meta?.placeholder != null) { + placeholderToDependency.set(dep.meta.placeholder, dep); + } } } @@ -206,15 +219,23 @@ export default class BundleGraph { let node = nullthrows(assetGraph.getNode(nodeId)); - if (node.type === 'asset') { + if (getFeatureFlag('conditionalBundling') && node.type === 'asset') { const asset = node.value; if (Array.isArray(asset.meta.conditions)) { for (const _condition of asset.meta.conditions ?? []) { const condition = String(_condition); + const [, ifTrueDep, ifFalseDep] = condition.split(':'); const condHash = hashString(condition); const condPublicId = getPublicId(condHash, v => conditions.has(v)); - // FIXME is this the right way around?? It is for packaging.. - conditions.set(condition, condPublicId); + + // FIXME - these dependencies exist in the AssetGraph, but don't + // seem to exist in the final BundleGraph - how do we map this properly?? + + conditions.set(condition, { + publicId: condPublicId, + ifTrueDependency: placeholderToDependency.get(ifTrueDep), + ifFalseDependency: placeholderToDependency.get(ifFalseDep), + }); } } } @@ -455,6 +476,7 @@ export default class BundleGraph { ); } } + return new BundleGraph({ graph, assetPublicIds, diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index f06fc538de1..435c25f4802 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -30,6 +30,7 @@ import Dependency, { import {targetToInternalTarget} from './Target'; import {fromInternalSourceLocation} from '../utils'; import BundleGroup, {bundleGroupToInternalBundleGroup} from './BundleGroup'; +import {getFeatureFlag} from '@parcel/feature-flags'; // Friendly access for other modules within this package that need access // to the internal bundle. @@ -328,20 +329,43 @@ export default class BundleGraph } getConditionPublicId(condition: string): string { + if (!getFeatureFlag('conditionalBundling')) { + throw new Error( + 'getConditionPublicId called but conditionalBundling is not enabled', + ); + } if (!this.#graph._conditions.has(condition)) { throw new Error(`Condition ${condition} was not in mapping`); } - return this.#graph._conditions.get(condition) ?? ''; + return this.#graph._conditions.get(condition)?.publicId ?? ''; } getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { + if (!getFeatureFlag('conditionalBundling')) { + throw new Error( + 'getCondtionMapping called but conditionalBundling is not enabled', + ); + } const ret = {}; - for (const [k, v] of this.#graph._conditions.entries()) { - const [feature, ifTruePlaceholder, ifFalsePlaceholder] = k.split(':'); - ret[v] = { + // console.log(this.#graph._conditions); + for (const [k, condition] of this.#graph._conditions.entries()) { + const [feature] = k.split(':'); + const [trueAsset, falseAsset] = [ + condition.ifTrueDependency, + condition.ifFalseDependency, + ].map(dep => { + const resolved = nullthrows(this.#graph.resolveAsyncDependency(dep)); + if (resolved.type === 'asset') { + return resolved.value; + } else { + return this.#graph.getAssetById(resolved.value.entryAssetId); + } + }); + + ret[condition.publicId] = { ff: feature, - t: ifTruePlaceholder, - f: ifFalsePlaceholder, + t: this.#graph.getAssetPublicId(trueAsset), + f: this.#graph.getAssetPublicId(falseAsset), }; } return ret; diff --git a/packages/core/core/src/public/MutableBundleGraph.js b/packages/core/core/src/public/MutableBundleGraph.js index 474d379eea0..2679a994f42 100644 --- a/packages/core/core/src/public/MutableBundleGraph.js +++ b/packages/core/core/src/public/MutableBundleGraph.js @@ -13,6 +13,7 @@ import type { ParcelOptions, BundleGroup as InternalBundleGroup, BundleNode, + Condition, } from '../types'; import invariant from 'assert'; @@ -38,7 +39,7 @@ export default class MutableBundleGraph #graph /*: InternalBundleGraph */; #options /*: ParcelOptions */; #bundlePublicIds /*: Set */ = new Set(); - #bundleConditions /* : Map */ = new Map(); + #bundleConditions /* : Map */ = new Map(); constructor(graph: InternalBundleGraph, options: ParcelOptions) { super(graph, Bundle.get.bind(Bundle), options); @@ -52,19 +53,39 @@ export default class MutableBundleGraph bundleToInternalBundle(bundle), ); - if (asset.meta.conditions != null && Array.isArray(asset.meta.conditions)) { - for (const _condition of asset.meta.conditions ?? []) { - const condition = String(_condition); - if (!this.#bundleConditions.has(condition)) { - const condHash = hashString(condition); - const condPublicId = getPublicId(condHash, v => - this.#bundleConditions.has(v), - ); - // FIXME is this the right way around?? It is for packaging.. - this.#bundleConditions.set(condition, condPublicId); - } - } - } + // TEST + // if (asset.meta.conditions != null && Array.isArray(asset.meta.conditions)) { + // for (const _condition of asset.meta.conditions ?? []) { + // const condition = String(_condition); + // if (!this.#bundleConditions.has(condition)) { + // const condHash = hashString(condition); + // const condPublicId = getPublicId(condHash, v => + // this.#bundleConditions.has(v), + // ); + + // const [, ifTrueDepPlaceholder, ifFalseDepPlaceholder] = condition.split(':'); + // let ifTrueDep, ifFalseDep; + // for (const dep of asset.getDependencies()) { + // if (dep.meta?.placeholder === ifTrueDepPlaceholder) { + // ifTrueDep = dependencyToInternalDependency(dep); + // } + // if (dep.meta?.placeholder === ifFalseDepPlaceholder) { + // ifFalseDep = dependencyToInternalDependency(dep); + // } + // } + // if (ifTrueDep == null || ifFalseDep == null) { + // throw new Error(`Deps were bad`); + // } + + // // FIXME is this the right way around?? It is for packaging.. + // this.#bundleConditions.set(condition, { + // publicId: condPublicId, + // ifTrueDependency: ifTrueDep, + // ifFalseDependency: ifFalseDep, + // }); + // } + // } + // } } addAssetGraphToBundle( @@ -333,7 +354,7 @@ export default class MutableBundleGraph ); } - getConditions(): Map { + getConditions(): Map { return this.#bundleConditions; } } diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js index 59353d85b9e..f972d479283 100644 --- a/packages/core/core/src/types.js +++ b/packages/core/core/src/types.js @@ -580,3 +580,9 @@ export type ValidationOpts = {| |}; export type ReportFn = (event: ReporterEvent) => void | Promise; + +export type Condition = {| + publicId: string, + ifTrueDependency: Dependency, + ifFalseDependency: Dependency, +|}; diff --git a/packages/core/feature-flags/src/index.js b/packages/core/feature-flags/src/index.js index c66b372d0dd..2bbe701c7ce 100644 --- a/packages/core/feature-flags/src/index.js +++ b/packages/core/feature-flags/src/index.js @@ -8,6 +8,7 @@ export type FeatureFlags = _FeatureFlags; export const DEFAULT_FEATURE_FLAGS: FeatureFlags = { exampleFeature: false, configKeyInvalidation: false, + conditionalBundling: false, }; let featureFlagValues: FeatureFlags = {...DEFAULT_FEATURE_FLAGS}; diff --git a/packages/core/feature-flags/src/types.js b/packages/core/feature-flags/src/types.js index 4edb1cf42f0..1ccd0a43205 100644 --- a/packages/core/feature-flags/src/types.js +++ b/packages/core/feature-flags/src/types.js @@ -9,4 +9,10 @@ export type FeatureFlags = {| * `config.getConfigFrom(..., {packageKey: '...'})` and the value itself hasn't changed. */ +configKeyInvalidation: boolean, + /** + * Enables experimental "conditional bundling" - this allows the use of `importCond` syntax + * in order to have (consumer) feature flag driven bundling. This feature is very experimental, + * and requires server-side support. + */ + +conditionalBundling: boolean, |}; diff --git a/packages/packagers/js/package.json b/packages/packagers/js/package.json index 81033c16210..6ea2cc74850 100644 --- a/packages/packagers/js/package.json +++ b/packages/packagers/js/package.json @@ -21,6 +21,7 @@ }, "dependencies": { "@parcel/diagnostic": "2.12.0", + "@parcel/feature-flags": "2.12.0", "@parcel/plugin": "2.12.0", "@parcel/rust": "2.12.0", "@parcel/source-map": "^2.1.1", diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index ae7c2786356..59d4b513d06 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -34,6 +34,7 @@ import { isValidIdentifier, makeValidIdentifier, } from './utils'; +import {getFeatureFlag} from '@parcel/feature-flags'; // General regex used to replace imports with the resolved code, references with resolutions, // and count the number of newlines in the file for source maps. @@ -617,8 +618,11 @@ export class ScopeHoistingPackager { return replacement; }); - // Handle conditional imports - if (code.includes('__parcel__requireCond__')) { + if ( + getFeatureFlag('conditionalBundling') && + code.includes('__parcel__requireCond__') + ) { + // Handle conditional imports console.log("Let's handle conditional imports!"); const IMPORT_COND_RE = /__parcel__requireCond__\(['"](.*?)['"]\)/g; code = code.replace(IMPORT_COND_RE, (m, s) => { @@ -1353,8 +1357,8 @@ ${code} lines += countLines(currentHelper) - 1; } } - - if (this.needsPrelude) { + // FIXME let's make all the bundles have the runtime with conditional bundling + if (this.needsPrelude || getFeatureFlag('conditionalBundling')) { // Add the prelude if this is potentially the first JS bundle to load in a // particular context (e.g. entry scripts in HTML, workers, etc.). let parentBundles = this.bundleGraph.getParentBundles(this.bundle); @@ -1365,7 +1369,8 @@ ${code} .getBundleGroupsContainingBundle(this.bundle) .some(g => this.bundleGraph.isEntryBundleGroup(g)) || this.bundle.env.isIsolated() || - this.bundle.bundleBehavior === 'isolated'; + this.bundle.bundleBehavior === 'isolated' || + getFeatureFlag('conditionalBundling'); if (mightBeFirstJS) { let preludeCode = prelude(this.parcelRequireName); diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index c6e6499f64b..0b348533337 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -19,6 +19,7 @@ import { import {encodeJSONKeyComponent} from '@parcel/diagnostic'; import path from 'path'; import nullthrows from 'nullthrows'; +import {getFeatureFlag} from '@parcel/feature-flags/src'; // Used for as="" in preload/prefetch const TYPE_TO_RESOURCE_PRIORITY = { @@ -660,13 +661,19 @@ function getRegisterCode( ? 'new __parcel__URL__("").toString()' // <-- this isn't ideal. We should use `import.meta.url` directly but it gets replaced currently : `require('./helpers/bundle-url').getBundleURL('${entryBundle.publicId}')`; - const conditions = bundleGraph.getConditionMapping(); + if (getFeatureFlag('conditionalBundling')) { + const conditions = bundleGraph.getConditionMapping(); - return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( - JSON.stringify(mappings), - )}));require('./helpers/bundle-manifest').registerConditions(JSON.parse(${JSON.stringify( - JSON.stringify(conditions), - )}));`; + return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( + JSON.stringify(mappings), + )}));parcelRequire.registerConditions(JSON.parse(${JSON.stringify( + JSON.stringify(conditions), + )}));`; + } else { + return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( + JSON.stringify(mappings), + )}));`; + } } function getRelativePathExpr( diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index 6c93909ff2a..9ed1c2c2bb4 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -458,7 +458,7 @@ impl<'a> Fold for DependencyCollector<'a> { )))); return call; } - "importCond" => DependencyKind::ConditionalImport, + "importCond" if self.config.conditional_bundling => DependencyKind::ConditionalImport, _ => return node.fold_children_with(self), } } @@ -703,7 +703,7 @@ impl<'a> Fold for DependencyCollector<'a> { } else if kind == DependencyKind::Require { // Don't continue traversing so that the `require` isn't replaced with undefined rewrite_require_specifier(node, self.unresolved_mark) - } else if kind == DependencyKind::ConditionalImport { + } else if self.config.conditional_bundling && kind == DependencyKind::ConditionalImport { let mut call = node; call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( "__parcel__requireCond__".into(), @@ -723,7 +723,7 @@ impl<'a> Fold for DependencyCollector<'a> { arg.span(), DependencyKind::ConditionalImport, None, - true, + false, self.config.source_type, ); placeholders.push(placeholder.unwrap()); diff --git a/packages/transformers/js/core/src/lib.rs b/packages/transformers/js/core/src/lib.rs index 7193205dff9..38ed655c1b6 100644 --- a/packages/transformers/js/core/src/lib.rs +++ b/packages/transformers/js/core/src/lib.rs @@ -93,6 +93,7 @@ pub struct Config { is_swc_helpers: bool, standalone: bool, inline_constants: bool, + conditional_bundling: bool, } #[derive(Serialize, Debug, Default)] diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index 2f1173a1627..1333398fd2d 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -460,6 +460,7 @@ export default (new Transformer({ is_swc_helpers: /@swc[/\\]helpers/.test(asset.filePath), standalone: asset.query.has('standalone'), inline_constants: config.inlineConstants, + conditional_bundling: options.featureFlags.conditionalBundling, callMacro: asset.isSource ? async (err, src, exportName, args, loc) => { let mod; @@ -890,6 +891,7 @@ export default (new Transformer({ .getDependencies() .map(dep => [dep.meta.placeholder ?? dep.specifier, dep]), ); + for (let dep of deps.values()) { dep.symbols.ensure(); } From 0a52318bcbe3486170488442929597ab453c4458 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Thu, 11 Apr 2024 15:06:02 +1000 Subject: [PATCH 03/14] Fix some flagging stuff --- packages/core/types-internal/src/index.js | 1 + .../packagers/js/src/ScopeHoistingPackager.js | 14 +++++- packages/packagers/js/src/helpers.js | 48 +++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/core/types-internal/src/index.js b/packages/core/types-internal/src/index.js index 6a4299a1bcd..3b2ac5032b7 100644 --- a/packages/core/types-internal/src/index.js +++ b/packages/core/types-internal/src/index.js @@ -1614,6 +1614,7 @@ export interface BundleGraph { /** Returns the common root directory for the entry assets of a target. */ getEntryRoot(target: Target): FilePath; getConditionPublicId(condition: string): string; + // getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}; } /** diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 59d4b513d06..b219dafa519 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -27,7 +27,13 @@ import path from 'path'; import {ESMOutputFormat} from './ESMOutputFormat'; import {CJSOutputFormat} from './CJSOutputFormat'; import {GlobalOutputFormat} from './GlobalOutputFormat'; -import {prelude, helpers, bundleQueuePrelude, fnExpr} from './helpers'; +import { + prelude, + conditionalBundlingPrelude, + helpers, + bundleQueuePrelude, + fnExpr, +} from './helpers'; import { replaceScriptDependencies, getSpecifier, @@ -1373,7 +1379,11 @@ ${code} getFeatureFlag('conditionalBundling'); if (mightBeFirstJS) { - let preludeCode = prelude(this.parcelRequireName); + let preludeCode = ( + getFeatureFlag('conditionalBundling') + ? conditionalBundlingPrelude + : prelude + )(this.parcelRequireName); res += preludeCode; if (enableSourceMaps) { lines += countLines(preludeCode) - 1; diff --git a/packages/packagers/js/src/helpers.js b/packages/packagers/js/src/helpers.js index de455c32c57..404323e3e09 100644 --- a/packages/packagers/js/src/helpers.js +++ b/packages/packagers/js/src/helpers.js @@ -1,6 +1,54 @@ // @flow strict-local import type {Environment} from '@parcel/types'; +export const conditionalBundlingPrelude = ( + parcelRequireName: string, +): string => ` +var $parcel$modules = {}; +var $parcel$inits = {}; +var $parcel$conditions = {}; + +var parcelRequire = $parcel$global[${JSON.stringify(parcelRequireName)}]; + +if (parcelRequire == null) { + parcelRequire = function(id) { + if (id.startsWith('cond:')) { + const condKey = id.slice(5); + if (!condKey in $parcel$conditions) { + throw new Error("Cannot find condition '" + condKey + "'"); + } + id = $parcel$conditions[condKey].t; + } + if (id in $parcel$modules) { + return $parcel$modules[id].exports; + } + if (id in $parcel$inits) { + var init = $parcel$inits[id]; + delete $parcel$inits[id]; + var module = {id: id, exports: {}}; + $parcel$modules[id] = module; + init.call(module.exports, module, module.exports); + return module.exports; + } + var err = new Error("Cannot find module '" + id + "'"); + err.code = 'MODULE_NOT_FOUND'; + throw err; + }; + + parcelRequire.register = function register(id, init) { + $parcel$inits[id] = init; + }; + + parcelRequire.registerConditions = function registerConditions(conditions) { + $parcel$conditions = conditions; + } + + $parcel$global[${JSON.stringify(parcelRequireName)}] = parcelRequire; +} + +var parcelRegister = parcelRequire.register; +`; + export const prelude = (parcelRequireName: string): string => ` var $parcel$modules = {}; var $parcel$inits = {}; From 2a0b5e65ba504b03ce93d96134c0a5b89c363c38 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Tue, 16 Apr 2024 08:50:56 +1000 Subject: [PATCH 04/14] WIP --- packages/core/core/src/BundleGraph.js | 3 +- packages/core/core/src/SymbolPropagation.js | 5 +- packages/core/core/src/public/BundleGraph.js | 31 +++++++++++ packages/core/core/src/types.js | 1 + packages/core/types-internal/src/index.js | 2 +- packages/packagers/js/src/DevPackager.js | 15 ++++++ .../packagers/js/src/ScopeHoistingPackager.js | 4 +- packages/packagers/js/src/dev-prelude.js | 8 +++ packages/runtimes/js/src/JSRuntime.js | 51 +++++++++++++------ .../js/core/src/dependency_collector.rs | 12 +++-- packages/transformers/js/core/src/hoist.rs | 31 ++++++++++- packages/transformers/js/core/src/utils.rs | 29 ++++++++++- packages/transformers/js/src/JSTransformer.js | 1 + 13 files changed, 167 insertions(+), 26 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 2c58b2917a0..fef889acc79 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -224,7 +224,7 @@ export default class BundleGraph { if (Array.isArray(asset.meta.conditions)) { for (const _condition of asset.meta.conditions ?? []) { const condition = String(_condition); - const [, ifTrueDep, ifFalseDep] = condition.split(':'); + const [key, ifTrueDep, ifFalseDep] = condition.split(':'); const condHash = hashString(condition); const condPublicId = getPublicId(condHash, v => conditions.has(v)); @@ -233,6 +233,7 @@ export default class BundleGraph { conditions.set(condition, { publicId: condPublicId, + key, ifTrueDependency: placeholderToDependency.get(ifTrueDep), ifFalseDependency: placeholderToDependency.get(ifFalseDep), }); diff --git a/packages/core/core/src/SymbolPropagation.js b/packages/core/core/src/SymbolPropagation.js index 7bac77187dd..b5876ccf08c 100644 --- a/packages/core/core/src/SymbolPropagation.js +++ b/packages/core/core/src/SymbolPropagation.js @@ -101,7 +101,10 @@ export function propagateSymbols({ namespaceReexportedSymbols.add('*'); } else { for (let incomingDep of incomingDeps) { - if (incomingDep.value.symbols == null) { + if ( + incomingDep.value.symbols == null || + incomingDep.value.priority === 3 + ) { if (incomingDep.value.sourceAssetId == null) { // The root dependency on non-library builds isEntry = true; diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index 435c25f4802..e91c15f227c 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -340,6 +340,37 @@ export default class BundleGraph return this.#graph._conditions.get(condition)?.publicId ?? ''; } + getConditionsForDependencies( + deps: Array, + ): Set<{|key: string, ifTrue: string, ifFalse: string|}> { + const conditions = new Set(); + const depIds = deps.map(dep => dep.id); + for (const [, condition] of this.#graph._conditions.entries()) { + if ( + depIds.includes(condition.ifTrueDependency.id) || + depIds.includes(condition.ifFalseDependency.id) + ) { + const [trueAsset, falseAsset] = [ + condition.ifTrueDependency, + condition.ifFalseDependency, + ].map(dep => { + const resolved = nullthrows(this.#graph.resolveAsyncDependency(dep)); + if (resolved.type === 'asset') { + return resolved.value; + } else { + return this.#graph.getAssetById(resolved.value.entryAssetId); + } + }); + conditions.add({ + key: condition.key, + ifTrue: this.#graph.getAssetPublicId(trueAsset), + ifFalse: this.#graph.getAssetPublicId(falseAsset), + }); + } + } + return conditions; + } + getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { if (!getFeatureFlag('conditionalBundling')) { throw new Error( diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js index f972d479283..a381fca4578 100644 --- a/packages/core/core/src/types.js +++ b/packages/core/core/src/types.js @@ -583,6 +583,7 @@ export type ReportFn = (event: ReporterEvent) => void | Promise; export type Condition = {| publicId: string, + key: string, ifTrueDependency: Dependency, ifFalseDependency: Dependency, |}; diff --git a/packages/core/types-internal/src/index.js b/packages/core/types-internal/src/index.js index 3b2ac5032b7..5c4835849c7 100644 --- a/packages/core/types-internal/src/index.js +++ b/packages/core/types-internal/src/index.js @@ -1614,7 +1614,7 @@ export interface BundleGraph { /** Returns the common root directory for the entry assets of a target. */ getEntryRoot(target: Target): FilePath; getConditionPublicId(condition: string): string; - // getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}; + getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}}; } /** diff --git a/packages/packagers/js/src/DevPackager.js b/packages/packagers/js/src/DevPackager.js index d0b5b5234b6..34256489672 100644 --- a/packages/packagers/js/src/DevPackager.js +++ b/packages/packagers/js/src/DevPackager.js @@ -8,6 +8,7 @@ import { normalizeSeparators, } from '@parcel/utils'; import SourceMap from '@parcel/source-map'; +import {getFeatureFlag} from '@parcel/feature-flags'; import invariant from 'assert'; import path from 'path'; import fs from 'fs'; @@ -114,6 +115,20 @@ export class DevPackager { let {code, mapBuffer} = results[i]; let output = code || ''; + if ( + getFeatureFlag('conditionalBundling') && + output.includes('__parcel__require__("cond:') + ) { + // FIXME move to utils - this regex also exists in ScopeHoistingPackager + output = output.replace( + /__parcel__require__\("cond:([^"]+)"\)/, + (s, longCondKey) => { + const condPublicId = + this.bundleGraph.getConditionPublicId(longCondKey); + return `require("./conditions/${condPublicId}.js")`; + }, + ); + } wrapped += JSON.stringify(this.bundleGraph.getAssetPublicId(asset)) + ':[function(require,module,exports) {\n' + diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index b219dafa519..feddfa02c5f 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -626,11 +626,11 @@ export class ScopeHoistingPackager { if ( getFeatureFlag('conditionalBundling') && - code.includes('__parcel__requireCond__') + code.includes('__parcel__require__') ) { // Handle conditional imports console.log("Let's handle conditional imports!"); - const IMPORT_COND_RE = /__parcel__requireCond__\(['"](.*?)['"]\)/g; + const IMPORT_COND_RE = /__parcel__require__\(['"]cond:(.*?)['"]\)/g; code = code.replace(IMPORT_COND_RE, (m, s) => { console.log(`_P_R_C_`, m, s); const condId = this.bundleGraph.getConditionPublicId(s); diff --git a/packages/packagers/js/src/dev-prelude.js b/packages/packagers/js/src/dev-prelude.js index 5636da413bd..7e836cfca71 100644 --- a/packages/packagers/js/src/dev-prelude.js +++ b/packages/packagers/js/src/dev-prelude.js @@ -109,6 +109,14 @@ {}, ]; }; + newRequire.conditions = previousRequire.conditions; + newRequire.registerConditions = function (conditions) { + for (var key in conditions) { + if (conditions.hasOwnProperty(key)) { + newRequire.conditions[key] = conditions[key]; + } + } + }; Object.defineProperty(newRequire, 'root', { get: function () { diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index 0b348533337..e2e651e4be0 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -19,7 +19,7 @@ import { import {encodeJSONKeyComponent} from '@parcel/diagnostic'; import path from 'path'; import nullthrows from 'nullthrows'; -import {getFeatureFlag} from '@parcel/feature-flags/src'; +import {getFeatureFlag} from '@parcel/feature-flags'; // Used for as="" in preload/prefetch const TYPE_TO_RESOURCE_PRIORITY = { @@ -67,6 +67,7 @@ let bundleDependencies = new WeakMap< NamedBundle, {| asyncDependencies: Array, + conditionalDependencies: Array, otherDependencies: Array, |}, >(); @@ -128,7 +129,8 @@ export default (new Runtime({ return; } - let {asyncDependencies, otherDependencies} = getDependencies(bundle); + let {asyncDependencies, conditionalDependencies, otherDependencies} = + getDependencies(bundle); let assets = []; for (let dependency of asyncDependencies) { @@ -187,6 +189,29 @@ export default (new Runtime({ } } + if (getFeatureFlag('conditionalBundling')) { + const conditions = bundleGraph.getConditionsForDependencies( + conditionalDependencies, + ); + for (const cond of conditions) { + const assetCode = `module.exports = window.__conditions['${cond.key}'] ? __parcel__require__('${cond.ifTrue}') : __parcel__require__('${cond.ifFalse}');`; + console.log(assetCode); + assets.push({ + filePath: path.join(__dirname, `/conditions/${cond.publicId}.js`), + code: assetCode, + dependency: conditionalDependencies[0], + env: {sourceType: 'module'}, + }); + } + } + // for (let dependency of conditionalDependencies) { + // console.log(dependency.id, dependency.meta); + // // assets.push({ + // // filePath. path.join(__dirname, `/bundles/${referencedBundle.id}.js`), + // // dependency, + // // }) + // } + for (let dependency of otherDependencies) { // Resolve the dependency to a bundle. If inline, export the dependency id, // which will be replaced with the contents of that bundle later. @@ -300,6 +325,7 @@ export default (new Runtime({ function getDependencies(bundle: NamedBundle): {| asyncDependencies: Array, + conditionalDependencies: Array, otherDependencies: Array, |} { let cachedDependencies = bundleDependencies.get(bundle); @@ -309,6 +335,7 @@ function getDependencies(bundle: NamedBundle): {| } else { let asyncDependencies = []; let otherDependencies = []; + let conditionalDependencies = []; bundle.traverse(node => { if (node.type !== 'dependency') { return; @@ -320,12 +347,14 @@ function getDependencies(bundle: NamedBundle): {| dependency.specifierType !== 'url' ) { asyncDependencies.push(dependency); + } else if (dependency.priority === 'conditional') { + conditionalDependencies.push(dependency); } else { otherDependencies.push(dependency); } }); bundleDependencies.set(bundle, {asyncDependencies, otherDependencies}); - return {asyncDependencies, otherDependencies}; + return {asyncDependencies, conditionalDependencies, otherDependencies}; } } @@ -661,19 +690,9 @@ function getRegisterCode( ? 'new __parcel__URL__("").toString()' // <-- this isn't ideal. We should use `import.meta.url` directly but it gets replaced currently : `require('./helpers/bundle-url').getBundleURL('${entryBundle.publicId}')`; - if (getFeatureFlag('conditionalBundling')) { - const conditions = bundleGraph.getConditionMapping(); - - return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( - JSON.stringify(mappings), - )}));parcelRequire.registerConditions(JSON.parse(${JSON.stringify( - JSON.stringify(conditions), - )}));`; - } else { - return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( - JSON.stringify(mappings), - )}));`; - } + return `require('./helpers/bundle-manifest').register(${baseUrl},JSON.parse(${JSON.stringify( + JSON.stringify(mappings), + )}));`; } function getRelativePathExpr( diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index 9ed1c2c2bb4..db54f50b095 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -367,6 +367,13 @@ impl<'a> Fold for DependencyCollector<'a> { Callee::Import(_) => DependencyKind::DynamicImport, Callee::Expr(expr) => { match &**expr { + // Handle this here becuase we want to treat importCond like it was a native Callee::Import + Ident(ident) + if self.config.conditional_bundling + && ident.sym.to_string().as_str() == "importCond" => + { + DependencyKind::ConditionalImport + } Ident(ident) => { // Bail if defined in scope if !is_unresolved(&ident, self.unresolved_mark) { @@ -458,7 +465,6 @@ impl<'a> Fold for DependencyCollector<'a> { )))); return call; } - "importCond" if self.config.conditional_bundling => DependencyKind::ConditionalImport, _ => return node.fold_children_with(self), } } @@ -706,7 +712,7 @@ impl<'a> Fold for DependencyCollector<'a> { } else if self.config.conditional_bundling && kind == DependencyKind::ConditionalImport { let mut call = node; call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( - "__parcel__requireCond__".into(), + "__parcel__require__".into(), DUMMY_SP, )))); @@ -741,7 +747,7 @@ impl<'a> Fold for DependencyCollector<'a> { call.args[0] = ast::ExprOrSpread { spread: None, expr: Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { - value: condition, + value: format!("cond:{}", condition).into(), span: DUMMY_SP, raw: None, }))), diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 6c701d39eff..69b1924cc2e 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -1,7 +1,7 @@ use crate::collect::{Collect, Export, Import, ImportKind}; use crate::utils::{ get_undefined_ident, is_unresolved, match_export_name, match_export_name_ident, - match_property_name, + match_import_cond, match_property_name, }; use indexmap::IndexMap; use serde::{Deserialize, Serialize}; @@ -696,6 +696,7 @@ impl<'a> Fold for Hoist<'a> { if let Some(source) = match_import(&node, self.collect.ignore_mark) { self.add_require(&source, ImportKind::DynamicImport); let name: JsWord = format!("${}$importAsync${:x}", self.module_id, hash!(source)).into(); + println!("Dynamic import for source {}", name); self.dynamic_imports.insert(name.clone(), source.clone()); if self.collect.non_static_requires.contains(&source) || self.collect.should_wrap { self.imported_symbols.push(ImportedSymbol { @@ -708,6 +709,34 @@ impl<'a> Fold for Hoist<'a> { } return Expr::Ident(Ident::new(name, call.span)); } + + // FIXME add match for importCond here? + if let Some((flag, dep_true, dep_false)) = match_import_cond(&node) { + println!("Conditional import: {} {}", dep_true, dep_false); + return Expr::Ident(Ident::new( + format!( + "{}$importCond${}", + self.module_id, + hash!(format!("{}:{}:{}", flag, dep_true, dep_false)) + ) + .into(), + call.span, + )); + // self.imported_symbols.push(ImportedSymbol { + // source: dep_true, + // local: "".into(), + // imported: "*".into(), + // loc: SourceLocation::from(&self.collect.source_map, call.span), + // kind: ImportKind::Import, + // }); + // self.imported_symbols.push(ImportedSymbol { + // source: dep_false, + // local: "".into(), + // imported: "*".into(), + // loc: SourceLocation::from(&self.collect.source_map, call.span), + // kind: ImportKind::Import, + // }); + } } Expr::This(this) => { if !self.in_function_scope { diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index aacd2bc5ea3..d2d6e84258f 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; use swc_core::common::errors::{DiagnosticBuilder, Emitter}; use swc_core::common::{Mark, SourceMap, Span, SyntaxContext, DUMMY_SP}; use swc_core::ecma::ast::{self, Ident}; -use swc_core::ecma::atoms::{js_word, JsWord}; +use swc_core::ecma::atoms::{js_word, Atom, JsWord}; pub fn is_unresolved(ident: &Ident, unresolved_mark: Mark) -> bool { ident.span.ctxt.outer() == unresolved_mark @@ -156,6 +156,33 @@ pub fn match_require(node: &ast::Expr, unresolved_mark: Mark, ignore_mark: Mark) } } +pub fn match_import_cond(node: &ast::Expr) -> Option<(Atom, Atom, Atom)> { + use ast::*; + + match node { + Expr::Call(call) => match &call.callee { + Callee::Expr(expr) => match &**expr { + Expr::Ident(ident) => { + if ident.sym == js_word!("__parcel__requireCond__") { + if !call.args.len() == 3 { + return None; + } + return Some(( + match_str(&call.args[0].expr).map(|(name, _)| name).unwrap(), + match_str(&call.args[1].expr).map(|(name, _)| name).unwrap(), + match_str(&call.args[2].expr).map(|(name, _)| name).unwrap(), + )); + } + None + } + _ => None, + }, + _ => None, + }, + _ => None, + } +} + pub fn match_import(node: &ast::Expr, ignore_mark: Mark) -> Option { use ast::*; diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index 1333398fd2d..306fb77e097 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -1065,6 +1065,7 @@ export default (new Transformer({ } asset.type = 'js'; + // console.log("Compiled code: " + compiledCode); asset.setBuffer(compiledCode); if (map) { From 36f66aa7f20390a3792310cea26186047dc4d6bb Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Wed, 17 Apr 2024 15:09:33 +1000 Subject: [PATCH 05/14] Fix and simplify a bunch of stuff --- packages/core/core/src/public/BundleGraph.js | 10 +++- packages/packagers/js/src/DevPackager.js | 14 ------ .../packagers/js/src/ScopeHoistingPackager.js | 28 +---------- packages/packagers/js/src/helpers.js | 48 ------------------- packages/packagers/js/src/utils.js | 1 - packages/runtimes/js/src/JSRuntime.js | 9 ++-- .../js/core/src/dependency_collector.rs | 22 +++++---- 7 files changed, 31 insertions(+), 101 deletions(-) diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index e91c15f227c..e66c580638b 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -342,7 +342,12 @@ export default class BundleGraph getConditionsForDependencies( deps: Array, - ): Set<{|key: string, ifTrue: string, ifFalse: string|}> { + ): Set<{| + key: string, + dependency: Dependency, + ifTrue: string, + ifFalse: string, + |}> { const conditions = new Set(); const depIds = deps.map(dep => dep.id); for (const [, condition] of this.#graph._conditions.entries()) { @@ -363,6 +368,9 @@ export default class BundleGraph }); conditions.add({ key: condition.key, + dependency: deps.find( + dep => dep.id === condition.ifTrueDependency.id, + ), ifTrue: this.#graph.getAssetPublicId(trueAsset), ifFalse: this.#graph.getAssetPublicId(falseAsset), }); diff --git a/packages/packagers/js/src/DevPackager.js b/packages/packagers/js/src/DevPackager.js index 34256489672..f14b5224c71 100644 --- a/packages/packagers/js/src/DevPackager.js +++ b/packages/packagers/js/src/DevPackager.js @@ -115,20 +115,6 @@ export class DevPackager { let {code, mapBuffer} = results[i]; let output = code || ''; - if ( - getFeatureFlag('conditionalBundling') && - output.includes('__parcel__require__("cond:') - ) { - // FIXME move to utils - this regex also exists in ScopeHoistingPackager - output = output.replace( - /__parcel__require__\("cond:([^"]+)"\)/, - (s, longCondKey) => { - const condPublicId = - this.bundleGraph.getConditionPublicId(longCondKey); - return `require("./conditions/${condPublicId}.js")`; - }, - ); - } wrapped += JSON.stringify(this.bundleGraph.getAssetPublicId(asset)) + ':[function(require,module,exports) {\n' + diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index feddfa02c5f..42987049d28 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -27,13 +27,7 @@ import path from 'path'; import {ESMOutputFormat} from './ESMOutputFormat'; import {CJSOutputFormat} from './CJSOutputFormat'; import {GlobalOutputFormat} from './GlobalOutputFormat'; -import { - prelude, - conditionalBundlingPrelude, - helpers, - bundleQueuePrelude, - fnExpr, -} from './helpers'; +import {prelude, helpers, bundleQueuePrelude, fnExpr} from './helpers'; import { replaceScriptDependencies, getSpecifier, @@ -623,20 +617,6 @@ export class ScopeHoistingPackager { } return replacement; }); - - if ( - getFeatureFlag('conditionalBundling') && - code.includes('__parcel__require__') - ) { - // Handle conditional imports - console.log("Let's handle conditional imports!"); - const IMPORT_COND_RE = /__parcel__require__\(['"]cond:(.*?)['"]\)/g; - code = code.replace(IMPORT_COND_RE, (m, s) => { - console.log(`_P_R_C_`, m, s); - const condId = this.bundleGraph.getConditionPublicId(s); - return `parcelRequire("cond:${condId}")`; - }); - } } // If the asset is wrapped, we need to insert the dependency code outside the parcelRequire.register @@ -1379,11 +1359,7 @@ ${code} getFeatureFlag('conditionalBundling'); if (mightBeFirstJS) { - let preludeCode = ( - getFeatureFlag('conditionalBundling') - ? conditionalBundlingPrelude - : prelude - )(this.parcelRequireName); + let preludeCode = prelude(this.parcelRequireName); res += preludeCode; if (enableSourceMaps) { lines += countLines(preludeCode) - 1; diff --git a/packages/packagers/js/src/helpers.js b/packages/packagers/js/src/helpers.js index 404323e3e09..de455c32c57 100644 --- a/packages/packagers/js/src/helpers.js +++ b/packages/packagers/js/src/helpers.js @@ -1,54 +1,6 @@ // @flow strict-local import type {Environment} from '@parcel/types'; -export const conditionalBundlingPrelude = ( - parcelRequireName: string, -): string => ` -var $parcel$modules = {}; -var $parcel$inits = {}; -var $parcel$conditions = {}; - -var parcelRequire = $parcel$global[${JSON.stringify(parcelRequireName)}]; - -if (parcelRequire == null) { - parcelRequire = function(id) { - if (id.startsWith('cond:')) { - const condKey = id.slice(5); - if (!condKey in $parcel$conditions) { - throw new Error("Cannot find condition '" + condKey + "'"); - } - id = $parcel$conditions[condKey].t; - } - if (id in $parcel$modules) { - return $parcel$modules[id].exports; - } - if (id in $parcel$inits) { - var init = $parcel$inits[id]; - delete $parcel$inits[id]; - var module = {id: id, exports: {}}; - $parcel$modules[id] = module; - init.call(module.exports, module, module.exports); - return module.exports; - } - var err = new Error("Cannot find module '" + id + "'"); - err.code = 'MODULE_NOT_FOUND'; - throw err; - }; - - parcelRequire.register = function register(id, init) { - $parcel$inits[id] = init; - }; - - parcelRequire.registerConditions = function registerConditions(conditions) { - $parcel$conditions = conditions; - } - - $parcel$global[${JSON.stringify(parcelRequireName)}] = parcelRequire; -} - -var parcelRegister = parcelRequire.register; -`; - export const prelude = (parcelRequireName: string): string => ` var $parcel$modules = {}; var $parcel$inits = {}; diff --git a/packages/packagers/js/src/utils.js b/packages/packagers/js/src/utils.js index b9cb88c41af..f16fcc8d3cf 100644 --- a/packages/packagers/js/src/utils.js +++ b/packages/packagers/js/src/utils.js @@ -25,7 +25,6 @@ export function replaceScriptDependencies( lineCount++; return '\n'; } - console.log(`_P_R_`, s); let dep = nullthrows(dependencies.find(d => getSpecifier(d) === s)); let resolved = nullthrows(bundleGraph.getResolvedAsset(dep, bundle)); let publicId = bundleGraph.getAssetPublicId(resolved); diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index e2e651e4be0..273da27fcbe 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -195,11 +195,10 @@ export default (new Runtime({ ); for (const cond of conditions) { const assetCode = `module.exports = window.__conditions['${cond.key}'] ? __parcel__require__('${cond.ifTrue}') : __parcel__require__('${cond.ifFalse}');`; - console.log(assetCode); assets.push({ filePath: path.join(__dirname, `/conditions/${cond.publicId}.js`), code: assetCode, - dependency: conditionalDependencies[0], + dependency: cond.dependency, env: {sourceType: 'module'}, }); } @@ -353,7 +352,11 @@ function getDependencies(bundle: NamedBundle): {| otherDependencies.push(dependency); } }); - bundleDependencies.set(bundle, {asyncDependencies, otherDependencies}); + bundleDependencies.set(bundle, { + asyncDependencies, + conditionalDependencies, + otherDependencies, + }); return {asyncDependencies, conditionalDependencies, otherDependencies}; } } diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index db54f50b095..3f95cee85ec 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -656,7 +656,7 @@ impl<'a> Fold for DependencyCollector<'a> { } if kind == DependencyKind::ConditionalImport { - println!("Conditional import for feature {}", specifier); + node } else { let placeholder = self.add_dependency( specifier, @@ -674,10 +674,11 @@ impl<'a> Fold for DependencyCollector<'a> { span, raw: None, }))); - return node; + node + } else { + node } } - node } else { node } @@ -712,7 +713,7 @@ impl<'a> Fold for DependencyCollector<'a> { } else if self.config.conditional_bundling && kind == DependencyKind::ConditionalImport { let mut call = node; call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( - "__parcel__require__".into(), + "require".into(), DUMMY_SP, )))); @@ -723,15 +724,19 @@ impl<'a> Fold for DependencyCollector<'a> { let mut placeholders = Vec::new(); for arg in &call.args[1..] { let specifier = match_str(&arg.expr).unwrap().0; - println!("Conditional specifier: {}", specifier); let placeholder = self.add_dependency( - specifier, + specifier.clone(), arg.span(), DependencyKind::ConditionalImport, None, false, self.config.source_type, ); + println!( + "Conditional specifier: {} -> {:?}", + specifier.clone(), + placeholder + ); placeholders.push(placeholder.unwrap()); } @@ -742,12 +747,13 @@ impl<'a> Fold for DependencyCollector<'a> { placeholders[1] ) .into(); - self.conditions.insert(condition.clone()); + self.conditions.insert(condition); call.args[0] = ast::ExprOrSpread { spread: None, expr: Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { - value: format!("cond:{}", condition).into(), + // We pick the "first" dependency here + value: format!("{}", placeholders[0]).into(), span: DUMMY_SP, raw: None, }))), From c6a42c0d26242e35c7a08ce4b92ab8c3b741da0d Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Wed, 17 Apr 2024 15:32:50 +1000 Subject: [PATCH 06/14] Remove extraneous log --- packages/transformers/js/core/src/hoist.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 69b1924cc2e..3a1e16b63eb 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -696,7 +696,6 @@ impl<'a> Fold for Hoist<'a> { if let Some(source) = match_import(&node, self.collect.ignore_mark) { self.add_require(&source, ImportKind::DynamicImport); let name: JsWord = format!("${}$importAsync${:x}", self.module_id, hash!(source)).into(); - println!("Dynamic import for source {}", name); self.dynamic_imports.insert(name.clone(), source.clone()); if self.collect.non_static_requires.contains(&source) || self.collect.should_wrap { self.imported_symbols.push(ImportedSymbol { From 05d33bf067c6889908b7bce3a882350706aa1199 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Wed, 17 Apr 2024 18:32:26 +1000 Subject: [PATCH 07/14] WIP --- packages/core/core/src/BundleGraph.js | 5 +- packages/core/core/src/public/BundleGraph.js | 48 ++++++++++++++++++-- packages/core/core/src/types.js | 1 + 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index fef889acc79..d2d661cfab3 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -228,11 +228,10 @@ export default class BundleGraph { const condHash = hashString(condition); const condPublicId = getPublicId(condHash, v => conditions.has(v)); - // FIXME - these dependencies exist in the AssetGraph, but don't - // seem to exist in the final BundleGraph - how do we map this properly?? - conditions.set(condition, { publicId: condPublicId, + // FIXME support the same condition used across multiple assets.. + assets: new Set([asset]), key, ifTrueDependency: placeholderToDependency.get(ifTrueDep), ifFalseDependency: placeholderToDependency.get(ifFalseDep), diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index e66c580638b..efe8c352bf0 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -340,9 +340,7 @@ export default class BundleGraph return this.#graph._conditions.get(condition)?.publicId ?? ''; } - getConditionsForDependencies( - deps: Array, - ): Set<{| + getConditionsForDependencies(deps: Array): Set<{| key: string, dependency: Dependency, ifTrue: string, @@ -379,6 +377,50 @@ export default class BundleGraph return conditions; } + getConditionsByBundle() { + // What do we want: + // we need the data so when we're rendering a link to bundle xyz, we know what conditions + // that bundle has, and which conditions depend on which other bundles? + const bundlesWithConditions = {}; + for (const condition of this.#graph._conditions.values()) { + for (const asset of condition.assets) { + const publicAsset = this.getAssetById(asset.id); + const bundlesWithAsset = this.getBundlesWithAsset(publicAsset); + for (const bundle of bundlesWithAsset) { + if (!Object.hasOwn(bundlesWithConditions, condition.key)) { + bundlesWithConditions[condition.key] = { + bundles: [], + }; + } + // FIXME - return more abstract data here - convert it in the reporter.. + bundlesWithConditions[condition.key].bundles.push(bundle.filePath); + + [ + {dep: condition.ifTrueDependency, state: 'ifTrue'}, + {dep: condition.ifFalseDependency, state: 'ifFalse'}, + ].forEach(({dep, state}) => { + const publicDep = this.getDependencies(publicAsset).find( + d => d.id === dep.id, + ); + const resolved = nullthrows(this.resolveAsyncDependency(publicDep)); + invariant(resolved.type === 'bundle_group'); + + const conditionBundles = nullthrows( + this.getBundlesInBundleGroup(resolved.value), + ); + if (!Object.hasOwn(bundlesWithConditions[condition.key], state)) { + bundlesWithConditions[condition.key][state] = {}; + } + bundlesWithConditions[condition.key][state] = conditionBundles + .reverse() + .map(b => b.filePath); + }); + } + } + } + return bundlesWithConditions; + } + getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { if (!getFeatureFlag('conditionalBundling')) { throw new Error( diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js index a381fca4578..848f90cc8a0 100644 --- a/packages/core/core/src/types.js +++ b/packages/core/core/src/types.js @@ -583,6 +583,7 @@ export type ReportFn = (event: ReporterEvent) => void | Promise; export type Condition = {| publicId: string, + assets: Set, key: string, ifTrueDependency: Dependency, ifFalseDependency: Dependency, From d938caab8b24cf922e03dbb2dd763328b2bfe1e0 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Thu, 18 Apr 2024 15:06:11 +1000 Subject: [PATCH 08/14] Work on making scope hoisiting build work --- packages/core/core/src/public/BundleGraph.js | 75 +++++++++---------- .../packagers/js/src/ScopeHoistingPackager.js | 18 ++++- packages/runtimes/js/src/JSRuntime.js | 12 +-- packages/transformers/js/core/src/collect.rs | 18 ++++- .../js/core/src/dependency_collector.rs | 10 ++- packages/transformers/js/core/src/hoist.rs | 45 +++++------ packages/transformers/js/core/src/utils.rs | 15 ++-- 7 files changed, 99 insertions(+), 94 deletions(-) diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index efe8c352bf0..d6905d22543 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -340,6 +340,7 @@ export default class BundleGraph return this.#graph._conditions.get(condition)?.publicId ?? ''; } + // FIXME improve these lookups getConditionsForDependencies(deps: Array): Set<{| key: string, dependency: Dependency, @@ -377,48 +378,46 @@ export default class BundleGraph return conditions; } - getConditionsByBundle() { - // What do we want: - // we need the data so when we're rendering a link to bundle xyz, we know what conditions - // that bundle has, and which conditions depend on which other bundles? - const bundlesWithConditions = {}; - for (const condition of this.#graph._conditions.values()) { - for (const asset of condition.assets) { - const publicAsset = this.getAssetById(asset.id); - const bundlesWithAsset = this.getBundlesWithAsset(publicAsset); + getConditionalBundleMapping(): {| + [string]: {| + bundlesWithCondition: Array, + ifTrueBundles: Array, + ifFalseBundles: Array, + |}, + |} { + let conditions = {}; + // Convert the internal references in conditions to public API references + for (const cond of this.#graph._conditions.values()) { + let assets = Array.from(cond.assets).map(asset => + nullthrows(this.getAssetById(asset.id)), + ); + let bundles = new Set(); + let ifTrueBundles = []; + let ifFalseBundles = []; + for (const asset of assets) { + const bundlesWithAsset = this.getBundlesWithAsset(asset); for (const bundle of bundlesWithAsset) { - if (!Object.hasOwn(bundlesWithConditions, condition.key)) { - bundlesWithConditions[condition.key] = { - bundles: [], - }; - } - // FIXME - return more abstract data here - convert it in the reporter.. - bundlesWithConditions[condition.key].bundles.push(bundle.filePath); - - [ - {dep: condition.ifTrueDependency, state: 'ifTrue'}, - {dep: condition.ifFalseDependency, state: 'ifFalse'}, - ].forEach(({dep, state}) => { - const publicDep = this.getDependencies(publicAsset).find( - d => d.id === dep.id, - ); - const resolved = nullthrows(this.resolveAsyncDependency(publicDep)); - invariant(resolved.type === 'bundle_group'); - - const conditionBundles = nullthrows( - this.getBundlesInBundleGroup(resolved.value), - ); - if (!Object.hasOwn(bundlesWithConditions[condition.key], state)) { - bundlesWithConditions[condition.key][state] = {}; - } - bundlesWithConditions[condition.key][state] = conditionBundles - .reverse() - .map(b => b.filePath); - }); + bundles.add(bundle); } + const assetDeps = this.getDependencies(asset); + const depToBundles = dep => { + const publicDep = nullthrows( + assetDeps.find(assetDep => dep.id === assetDep.id), + ); + const resolved = nullthrows(this.resolveAsyncDependency(publicDep)); + invariant(resolved.type === 'bundle_group'); + return this.getBundlesInBundleGroup(resolved.value); + }; + ifTrueBundles.push(...depToBundles(cond.ifTrueDependency)); + ifFalseBundles.push(...depToBundles(cond.ifFalseDependency)); } + conditions[cond.key] = { + bundlesWithCondition: Array.from(bundles), + ifTrueBundles, + ifFalseBundles, + }; } - return bundlesWithConditions; + return conditions; } getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 42987049d28..3b71f6db218 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -39,7 +39,7 @@ import {getFeatureFlag} from '@parcel/feature-flags'; // General regex used to replace imports with the resolved code, references with resolutions, // and count the number of newlines in the file for source maps. const REPLACEMENT_RE = - /\n|import\s+"([0-9a-f]{16}:.+?)";|(?:\$[0-9a-f]{16}\$exports)|(?:\$[0-9a-f]{16}\$(?:import|importAsync|require)\$[0-9a-f]+(?:\$[0-9a-f]+)?)/g; + /\n|import\s+"([0-9a-f]{16}:.+?)";|(?:\$[0-9a-f]{16}\$exports)|(?:\$[0-9a-f]{16}\$(?:import|importAsync|importCond|require)\$[0-9a-f]+(?:\$[0-9a-f]+)?)/g; const BUILTINS = Object.keys(globals.builtin); const GLOBALS_BY_CONTEXT = { @@ -537,6 +537,8 @@ export class ScopeHoistingPackager { lineCount++; return '\n'; } + if (m.includes('importCond') || m.includes('importAsync')) + console.log(m, d, i); // If we matched an import, replace with the source code for the dependency. if (d != null) { @@ -701,7 +703,6 @@ ${code} if (!resolved) { continue; } - // Handle imports from other bundles in libraries. if (this.bundle.env.isLibrary && !this.bundle.hasAsset(resolved)) { let referencedBundle = this.bundleGraph.getReferencedBundle( @@ -721,11 +722,16 @@ ${code} } for (let [imported, {local}] of dep.symbols) { + if (dep.priority === 'conditional') { + console.log(dep.id, resolved, imported, local); + } + if (local === '*') { continue; } let symbol = this.getSymbolResolution(asset, resolved, imported, dep); + if (dep.priority === 'conditional') console.log('\t', symbol); replacements.set( local, // If this was an internalized async asset, wrap in a Promise.resolve. @@ -738,7 +744,10 @@ ${code} // Async dependencies need a namespace object even if all used symbols were statically analyzed. // This is recorded in the promiseSymbol meta property set by the transformer rather than in // symbols so that we don't mark all symbols as used. - if (dep.priority === 'lazy' && dep.meta.promiseSymbol) { + if ( + (dep.priority === 'lazy' || dep.priority === 'conditional') && + dep.meta.promiseSymbol + ) { let promiseSymbol = dep.meta.promiseSymbol; invariant(typeof promiseSymbol === 'string'); let symbol = this.getSymbolResolution(asset, resolved, '*', dep); @@ -933,6 +942,9 @@ ${code} symbol, } = this.bundleGraph.getSymbolResolution(resolved, imported, this.bundle); + if (dep?.priority === 'conditional' || dep?.priority === 'lazy') { + debugger; + } if ( resolvedAsset.type !== 'js' || (dep && this.bundleGraph.isDependencySkipped(dep)) diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index 273da27fcbe..14fad35cd5f 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -194,7 +194,10 @@ export default (new Runtime({ conditionalDependencies, ); for (const cond of conditions) { - const assetCode = `module.exports = window.__conditions['${cond.key}'] ? __parcel__require__('${cond.ifTrue}') : __parcel__require__('${cond.ifFalse}');`; + const requireName = bundle.env.shouldScopeHoist + ? 'parcelRequire' + : '__parcel__require__'; + const assetCode = `module.exports = globalThis.__conditions['${cond.key}'] ? ${requireName}('${cond.ifTrue}') : ${requireName}('${cond.ifFalse}');`; assets.push({ filePath: path.join(__dirname, `/conditions/${cond.publicId}.js`), code: assetCode, @@ -203,13 +206,6 @@ export default (new Runtime({ }); } } - // for (let dependency of conditionalDependencies) { - // console.log(dependency.id, dependency.meta); - // // assets.push({ - // // filePath. path.join(__dirname, `/bundles/${referencedBundle.id}.js`), - // // dependency, - // // }) - // } for (let dependency of otherDependencies) { // Resolve the dependency to a bundle. If inline, export the dependency id, diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 3bf6c2ddf12..89dc811d7d0 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -1,7 +1,7 @@ use crate::id; use crate::utils::{ - is_unresolved, match_export_name, match_export_name_ident, match_import, match_member_expr, - match_property_name, match_require, Bailout, BailoutReason, SourceLocation, + is_unresolved, match_export_name, match_export_name_ident, match_import, match_import_cond, + match_member_expr, match_property_name, match_require, Bailout, BailoutReason, SourceLocation, }; use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; @@ -29,6 +29,7 @@ pub enum ImportKind { Require, Import, DynamicImport, + ConditionalImport, } #[derive(Debug)] @@ -746,6 +747,15 @@ impl Visit for Collect { self.add_bailout(span, BailoutReason::NonStaticDynamicImport); } + if let Some(source) = match_import_cond(node, self.ignore_mark) { + self.wrapped_requires.insert(source.to_string()); + let span = match node { + Expr::Call(c) => c.span, + _ => unreachable!(), + }; + self.add_bailout(span, BailoutReason::NonStaticDynamicImport); + } + match node { Expr::Ident(ident) => { // Bail if `module` or `exports` are accessed non-statically. @@ -959,11 +969,11 @@ impl Collect { ImportKind::Import => self .wrapped_requires .insert(format!("{}{}", src.clone(), "esm")), - ImportKind::DynamicImport | ImportKind::Require => { + ImportKind::DynamicImport | ImportKind::Require | ImportKind::ConditionalImport => { self.wrapped_requires.insert(src.to_string()) } }; - if kind != ImportKind::DynamicImport { + if kind != ImportKind::DynamicImport && kind != ImportKind::ConditionalImport { self.non_static_requires.insert(src.clone()); let span = match node { Pat::Ident(id) => id.id.span, diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index 3f95cee85ec..b54442dc186 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -712,10 +712,12 @@ impl<'a> Fold for DependencyCollector<'a> { rewrite_require_specifier(node, self.unresolved_mark) } else if self.config.conditional_bundling && kind == DependencyKind::ConditionalImport { let mut call = node; - call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( - "require".into(), - DUMMY_SP, - )))); + if !self.config.scope_hoist { + call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( + "require".into(), + DUMMY_SP, + )))); + } if call.args.len() != 3 { // FIXME make this a diagnostic diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 3a1e16b63eb..49853ff17ee 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -709,32 +709,21 @@ impl<'a> Fold for Hoist<'a> { return Expr::Ident(Ident::new(name, call.span)); } - // FIXME add match for importCond here? - if let Some((flag, dep_true, dep_false)) = match_import_cond(&node) { - println!("Conditional import: {} {}", dep_true, dep_false); - return Expr::Ident(Ident::new( - format!( - "{}$importCond${}", - self.module_id, - hash!(format!("{}:{}:{}", flag, dep_true, dep_false)) - ) - .into(), - call.span, - )); - // self.imported_symbols.push(ImportedSymbol { - // source: dep_true, - // local: "".into(), - // imported: "*".into(), - // loc: SourceLocation::from(&self.collect.source_map, call.span), - // kind: ImportKind::Import, - // }); - // self.imported_symbols.push(ImportedSymbol { - // source: dep_false, - // local: "".into(), - // imported: "*".into(), - // loc: SourceLocation::from(&self.collect.source_map, call.span), - // kind: ImportKind::Import, - // }); + // FIXME add match for importCond here? Treat it similar to above.. + if let Some(source) = match_import_cond(&node, self.collect.ignore_mark) { + println!("Hoist conditional import -> {}", source); + let name: JsWord = format!("${}$importCond${}", self.module_id, hash!(source)).into(); + self.add_require(&source, ImportKind::ConditionalImport); + // ???? + self.dynamic_imports.insert(name.clone(), source.clone()); + self.imported_symbols.push(ImportedSymbol { + source, + local: name.clone(), + imported: "*".into(), + loc: SourceLocation::from(&self.collect.source_map, call.span), + kind: ImportKind::ConditionalImport, + }); + return Expr::Ident(Ident::new(name, call.span)); } } Expr::This(this) => { @@ -1028,7 +1017,9 @@ impl<'a> Hoist<'a> { fn add_require(&mut self, source: &JsWord, import_kind: ImportKind) { let src = match import_kind { ImportKind::Import => format!("{}:{}:{}", self.module_id, source, "esm"), - ImportKind::DynamicImport | ImportKind::Require => format!("{}:{}", self.module_id, source), + ImportKind::DynamicImport | ImportKind::Require | ImportKind::ConditionalImport => { + format!("{}:{}", self.module_id, source) + } }; self .module_items diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index d2d6e84258f..cf35dfffc78 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; use swc_core::common::errors::{DiagnosticBuilder, Emitter}; use swc_core::common::{Mark, SourceMap, Span, SyntaxContext, DUMMY_SP}; use swc_core::ecma::ast::{self, Ident}; -use swc_core::ecma::atoms::{js_word, Atom, JsWord}; +use swc_core::ecma::atoms::{js_word, JsWord}; pub fn is_unresolved(ident: &Ident, unresolved_mark: Mark) -> bool { ident.span.ctxt.outer() == unresolved_mark @@ -156,22 +156,17 @@ pub fn match_require(node: &ast::Expr, unresolved_mark: Mark, ignore_mark: Mark) } } -pub fn match_import_cond(node: &ast::Expr) -> Option<(Atom, Atom, Atom)> { +pub fn match_import_cond(node: &ast::Expr, ignore_mark: Mark) -> Option { use ast::*; match node { Expr::Call(call) => match &call.callee { Callee::Expr(expr) => match &**expr { Expr::Ident(ident) => { - if ident.sym == js_word!("__parcel__requireCond__") { - if !call.args.len() == 3 { - return None; + if ident.sym == js_word!("importCond") && !is_marked(ident.span, ignore_mark) { + if let Some(arg) = call.args.first() { + return match_str(&arg.expr).map(|(name, _)| name); } - return Some(( - match_str(&call.args[0].expr).map(|(name, _)| name).unwrap(), - match_str(&call.args[1].expr).map(|(name, _)| name).unwrap(), - match_str(&call.args[2].expr).map(|(name, _)| name).unwrap(), - )); } None } From fdb9134546de2a589dd71904dbca6102f2c7de50 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Thu, 18 Apr 2024 16:53:23 +1000 Subject: [PATCH 09/14] Hacks, but make ScopeHoistingPackager work! --- .../packagers/js/src/ScopeHoistingPackager.js | 125 ++++++++++-------- packages/transformers/js/core/src/collect.rs | 5 +- .../js/core/src/dependency_collector.rs | 13 +- packages/transformers/js/core/src/hoist.rs | 31 ++++- packages/transformers/js/core/src/utils.rs | 16 ++- 5 files changed, 119 insertions(+), 71 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 3b71f6db218..fd14aa4fd09 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -111,8 +111,6 @@ export class ScopeHoistingPackager { this.parcelRequireName = parcelRequireName; this.useAsyncBundleRuntime = useAsyncBundleRuntime; - // console.log("CONDTIONS", bundleGraph.getConditions()); - let OutputFormat = OUTPUT_FORMATS[this.bundle.env.outputFormat]; this.outputFormat = new OutputFormat(this); @@ -538,69 +536,72 @@ export class ScopeHoistingPackager { return '\n'; } if (m.includes('importCond') || m.includes('importAsync')) - console.log(m, d, i); + if (d != null) { + // console.log(`mdi`, m, d, i); - // If we matched an import, replace with the source code for the dependency. - if (d != null) { - let deps = depMap.get(d); - if (!deps) { - return m; - } + // If we matched an import, replace with the source code for the dependency. + let deps = depMap.get(d); + if (!deps) { + return m; + } - let replacement = ''; - - // A single `${id}:${specifier}:esm` might have been resolved to multiple assets due to - // reexports. - for (let dep of deps) { - let resolved = this.bundleGraph.getResolvedAsset(dep, this.bundle); - let skipped = this.bundleGraph.isDependencySkipped(dep); - if (resolved && !skipped) { - // Hoist variable declarations for the referenced parcelRequire dependencies - // after the dependency is declared. This handles the case where the resulting asset - // is wrapped, but the dependency in this asset is not marked as wrapped. This means - // that it was imported/required at the top-level, so its side effects should run immediately. - let [res, lines] = this.getHoistedParcelRequires( - asset, + let replacement = ''; + + // A single `${id}:${specifier}:esm` might have been resolved to multiple assets due to + // reexports. + for (let dep of deps) { + let resolved = this.bundleGraph.getResolvedAsset( dep, - resolved, + this.bundle, ); - let map; - if ( - this.bundle.hasAsset(resolved) && - !this.seenAssets.has(resolved.id) - ) { - // If this asset is wrapped, we need to hoist the code for the dependency - // outside our parcelRequire.register wrapper. This is safe because all - // assets referenced by this asset will also be wrapped. Otherwise, inline the - // asset content where the import statement was. - if (shouldWrap) { - depContent.push(this.visitAsset(resolved)); - } else { - let [depCode, depMap, depLines] = this.visitAsset(resolved); - res = depCode + '\n' + res; - lines += 1 + depLines; - map = depMap; + let skipped = this.bundleGraph.isDependencySkipped(dep); + if (resolved && !skipped) { + // Hoist variable declarations for the referenced parcelRequire dependencies + // after the dependency is declared. This handles the case where the resulting asset + // is wrapped, but the dependency in this asset is not marked as wrapped. This means + // that it was imported/required at the top-level, so its side effects should run immediately. + let [res, lines] = this.getHoistedParcelRequires( + asset, + dep, + resolved, + ); + let map; + if ( + this.bundle.hasAsset(resolved) && + !this.seenAssets.has(resolved.id) + ) { + // If this asset is wrapped, we need to hoist the code for the dependency + // outside our parcelRequire.register wrapper. This is safe because all + // assets referenced by this asset will also be wrapped. Otherwise, inline the + // asset content where the import statement was. + if (shouldWrap) { + depContent.push(this.visitAsset(resolved)); + } else { + let [depCode, depMap, depLines] = this.visitAsset(resolved); + res = depCode + '\n' + res; + lines += 1 + depLines; + map = depMap; + } } - } - // Push this asset's source mappings down by the number of lines in the dependency - // plus the number of hoisted parcelRequires. Then insert the source map for the dependency. - if (sourceMap) { - if (lines > 0) { - sourceMap.offsetLines(lineCount + 1, lines); - } + // Push this asset's source mappings down by the number of lines in the dependency + // plus the number of hoisted parcelRequires. Then insert the source map for the dependency. + if (sourceMap) { + if (lines > 0) { + sourceMap.offsetLines(lineCount + 1, lines); + } - if (map) { - sourceMap.addSourceMap(map, lineCount); + if (map) { + sourceMap.addSourceMap(map, lineCount); + } } - } - replacement += res; - lineCount += lines; + replacement += res; + lineCount += lines; + } } + return replacement; } - return replacement; - } // If it wasn't a dependency, then it was an inline replacement (e.g. $id$import$foo -> $id$export$foo). let replacement = replacements.get(m) ?? m; @@ -723,7 +724,7 @@ ${code} for (let [imported, {local}] of dep.symbols) { if (dep.priority === 'conditional') { - console.log(dep.id, resolved, imported, local); + // console.log(`dep.symbols:`, dep.id, resolved, imported, local); } if (local === '*') { @@ -731,7 +732,12 @@ ${code} } let symbol = this.getSymbolResolution(asset, resolved, imported, dep); - if (dep.priority === 'conditional') console.log('\t', symbol); + // FIXME lol + if (dep.priority === 'conditional' && replacements.has(local)) { + // console.log('\t', symbol); + continue; + } + // console.log(`replacement dep.symbols ${local} -> ${symbol}`); replacements.set( local, // If this was an internalized async asset, wrap in a Promise.resolve. @@ -751,6 +757,11 @@ ${code} let promiseSymbol = dep.meta.promiseSymbol; invariant(typeof promiseSymbol === 'string'); let symbol = this.getSymbolResolution(asset, resolved, '*', dep); + // console.log(`replacement promiseSymbol ${promiseSymbol} -> ${symbol}`); + // FIXME lol + if (dep.priority === 'conditional' && replacements.has(promiseSymbol)) { + continue; + } replacements.set( promiseSymbol, asyncResolution?.type === 'asset' @@ -812,6 +823,7 @@ ${code} // If already imported, just add the already renamed variable to the mapping. let renamed = external.get(imported); if (renamed && local !== '*' && replacements) { + // console.log(`replacement renamed ${local} -> ${renamed}`); replacements.set(local, renamed); continue; } @@ -909,6 +921,7 @@ ${code} } else if (property) { replacement = this.getPropertyAccess(renamed, property); } + // console.log(`replacement at the bottom ${local} -> ${replacement}`); replacements.set(local, replacement); } } diff --git a/packages/transformers/js/core/src/collect.rs b/packages/transformers/js/core/src/collect.rs index 89dc811d7d0..949b12066f1 100644 --- a/packages/transformers/js/core/src/collect.rs +++ b/packages/transformers/js/core/src/collect.rs @@ -747,8 +747,9 @@ impl Visit for Collect { self.add_bailout(span, BailoutReason::NonStaticDynamicImport); } - if let Some(source) = match_import_cond(node, self.ignore_mark) { - self.wrapped_requires.insert(source.to_string()); + if let Some((source_true, source_false)) = match_import_cond(node, self.ignore_mark) { + self.wrapped_requires.insert(source_true.to_string()); + self.wrapped_requires.insert(source_false.to_string()); let span = match node { Expr::Call(c) => c.span, _ => unreachable!(), diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index b54442dc186..b14361437ee 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -751,16 +751,25 @@ impl<'a> Fold for DependencyCollector<'a> { .into(); self.conditions.insert(condition); + // write out code like importCond(depIfTrue, depIfFalse) - while we use the first dep as the link to the conditions + // we need both deps to ensure scope hoisting can make sure both arms are treated as "used" call.args[0] = ast::ExprOrSpread { spread: None, expr: Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { - // We pick the "first" dependency here value: format!("{}", placeholders[0]).into(), span: DUMMY_SP, raw: None, }))), }; - call.args.truncate(1); + call.args[1] = ast::ExprOrSpread { + spread: None, + expr: Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str { + value: format!("{}", placeholders[1]).into(), + span: DUMMY_SP, + raw: None, + }))), + }; + call.args.truncate(2); call } else { diff --git a/packages/transformers/js/core/src/hoist.rs b/packages/transformers/js/core/src/hoist.rs index 49853ff17ee..3820fb6beeb 100644 --- a/packages/transformers/js/core/src/hoist.rs +++ b/packages/transformers/js/core/src/hoist.rs @@ -710,14 +710,33 @@ impl<'a> Fold for Hoist<'a> { } // FIXME add match for importCond here? Treat it similar to above.. - if let Some(source) = match_import_cond(&node, self.collect.ignore_mark) { - println!("Hoist conditional import -> {}", source); - let name: JsWord = format!("${}$importCond${}", self.module_id, hash!(source)).into(); - self.add_require(&source, ImportKind::ConditionalImport); + if let Some((source_true, source_false)) = + match_import_cond(&node, self.collect.ignore_mark) + { + println!( + "Hoist conditional import -> {},{}", + source_true, source_false + ); + let name: JsWord = + format!("${}$importCond${}", self.module_id, hash!(source_true)).into(); + self.add_require(&source_true, ImportKind::ConditionalImport); + self.add_require(&source_false, ImportKind::ConditionalImport); // ???? - self.dynamic_imports.insert(name.clone(), source.clone()); + self + .dynamic_imports + .insert(name.clone(), source_true.clone()); + self + .dynamic_imports + .insert(name.clone(), source_false.clone()); + self.imported_symbols.push(ImportedSymbol { + source: source_true, + local: name.clone(), + imported: "*".into(), + loc: SourceLocation::from(&self.collect.source_map, call.span), + kind: ImportKind::ConditionalImport, + }); self.imported_symbols.push(ImportedSymbol { - source, + source: source_false, local: name.clone(), imported: "*".into(), loc: SourceLocation::from(&self.collect.source_map, call.span), diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index cf35dfffc78..995e4fd14e0 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -156,17 +156,23 @@ pub fn match_require(node: &ast::Expr, unresolved_mark: Mark, ignore_mark: Mark) } } -pub fn match_import_cond(node: &ast::Expr, ignore_mark: Mark) -> Option { +pub fn match_import_cond(node: &ast::Expr, ignore_mark: Mark) -> Option<(JsWord, JsWord)> { use ast::*; match node { Expr::Call(call) => match &call.callee { Callee::Expr(expr) => match &**expr { Expr::Ident(ident) => { - if ident.sym == js_word!("importCond") && !is_marked(ident.span, ignore_mark) { - if let Some(arg) = call.args.first() { - return match_str(&arg.expr).map(|(name, _)| name); - } + if ident.sym == js_word!("importCond") + && !is_marked(ident.span, ignore_mark) + && call.args.len() == 2 + { + let if_true = match_str(&call.args[0].expr).map(|(name, _)| name); + let if_false = match_str(&call.args[1].expr).map(|(name, _)| name); + return match (if_true, if_false) { + (Some(if_true), Some(if_false)) => Some((if_true, if_false)), + _ => None, + }; } None } From b97b2d9322ccf7f7ea5e743c55158d86f114071d Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Fri, 19 Apr 2024 09:45:32 +1000 Subject: [PATCH 10/14] Remove bad indent --- .../packagers/js/src/ScopeHoistingPackager.js | 109 +++++++++--------- 1 file changed, 53 insertions(+), 56 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index fd14aa4fd09..3e22f64df2a 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -535,73 +535,70 @@ export class ScopeHoistingPackager { lineCount++; return '\n'; } - if (m.includes('importCond') || m.includes('importAsync')) - if (d != null) { - // console.log(`mdi`, m, d, i); - - // If we matched an import, replace with the source code for the dependency. - let deps = depMap.get(d); - if (!deps) { - return m; - } - let replacement = ''; + if (d != null) { + // console.log(`mdi`, m, d, i); + + // If we matched an import, replace with the source code for the dependency. + let deps = depMap.get(d); + if (!deps) { + return m; + } - // A single `${id}:${specifier}:esm` might have been resolved to multiple assets due to - // reexports. - for (let dep of deps) { - let resolved = this.bundleGraph.getResolvedAsset( + let replacement = ''; + + // A single `${id}:${specifier}:esm` might have been resolved to multiple assets due to + // reexports. + for (let dep of deps) { + let resolved = this.bundleGraph.getResolvedAsset(dep, this.bundle); + let skipped = this.bundleGraph.isDependencySkipped(dep); + if (resolved && !skipped) { + // Hoist variable declarations for the referenced parcelRequire dependencies + // after the dependency is declared. This handles the case where the resulting asset + // is wrapped, but the dependency in this asset is not marked as wrapped. This means + // that it was imported/required at the top-level, so its side effects should run immediately. + let [res, lines] = this.getHoistedParcelRequires( + asset, dep, - this.bundle, + resolved, ); - let skipped = this.bundleGraph.isDependencySkipped(dep); - if (resolved && !skipped) { - // Hoist variable declarations for the referenced parcelRequire dependencies - // after the dependency is declared. This handles the case where the resulting asset - // is wrapped, but the dependency in this asset is not marked as wrapped. This means - // that it was imported/required at the top-level, so its side effects should run immediately. - let [res, lines] = this.getHoistedParcelRequires( - asset, - dep, - resolved, - ); - let map; - if ( - this.bundle.hasAsset(resolved) && - !this.seenAssets.has(resolved.id) - ) { - // If this asset is wrapped, we need to hoist the code for the dependency - // outside our parcelRequire.register wrapper. This is safe because all - // assets referenced by this asset will also be wrapped. Otherwise, inline the - // asset content where the import statement was. - if (shouldWrap) { - depContent.push(this.visitAsset(resolved)); - } else { - let [depCode, depMap, depLines] = this.visitAsset(resolved); - res = depCode + '\n' + res; - lines += 1 + depLines; - map = depMap; - } + let map; + if ( + this.bundle.hasAsset(resolved) && + !this.seenAssets.has(resolved.id) + ) { + // If this asset is wrapped, we need to hoist the code for the dependency + // outside our parcelRequire.register wrapper. This is safe because all + // assets referenced by this asset will also be wrapped. Otherwise, inline the + // asset content where the import statement was. + if (shouldWrap) { + depContent.push(this.visitAsset(resolved)); + } else { + let [depCode, depMap, depLines] = this.visitAsset(resolved); + res = depCode + '\n' + res; + lines += 1 + depLines; + map = depMap; } + } - // Push this asset's source mappings down by the number of lines in the dependency - // plus the number of hoisted parcelRequires. Then insert the source map for the dependency. - if (sourceMap) { - if (lines > 0) { - sourceMap.offsetLines(lineCount + 1, lines); - } - - if (map) { - sourceMap.addSourceMap(map, lineCount); - } + // Push this asset's source mappings down by the number of lines in the dependency + // plus the number of hoisted parcelRequires. Then insert the source map for the dependency. + if (sourceMap) { + if (lines > 0) { + sourceMap.offsetLines(lineCount + 1, lines); } - replacement += res; - lineCount += lines; + if (map) { + sourceMap.addSourceMap(map, lineCount); + } } + + replacement += res; + lineCount += lines; } - return replacement; } + return replacement; + } // If it wasn't a dependency, then it was an inline replacement (e.g. $id$import$foo -> $id$export$foo). let replacement = replacements.get(m) ?? m; From 662afcfe5da082e123d4e75243a268db1024458d Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Fri, 19 Apr 2024 10:10:51 +1000 Subject: [PATCH 11/14] Document a bunch of things, remove dead code --- packages/core/core/src/BundleGraph.js | 6 +++ packages/core/core/src/public/BundleGraph.js | 51 +++++-------------- .../core/src/public/MutableBundleGraph.js | 39 -------------- packages/core/types-internal/src/index.js | 10 +++- .../packagers/js/src/ScopeHoistingPackager.js | 7 ++- packages/packagers/js/src/dev-prelude.js | 8 --- packages/packagers/js/src/utils.js | 1 + packages/runtimes/js/src/JSRuntime.js | 3 ++ .../js/src/helpers/bundle-manifest.js | 8 --- 9 files changed, 35 insertions(+), 98 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index d2d661cfab3..4856b90b245 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -205,6 +205,9 @@ export default class BundleGraph { node != null && node.type === 'dependency' ) { + // The dependency placeholders in the `importCond` calls that will be in the transformed + // code need to be mapped to the "real" depenencies, so we need access to a map of placeholders + // to dependencies const dep = node.value; if (dep.meta?.placeholder != null) { placeholderToDependency.set(dep.meta.placeholder, dep); @@ -223,6 +226,9 @@ export default class BundleGraph { const asset = node.value; if (Array.isArray(asset.meta.conditions)) { for (const _condition of asset.meta.conditions ?? []) { + // Asset meta conditions will be of the form `key:placeholder_if_true:placeholfder_if_false` + // Here we extract that information and create `Condition`s which resolve those placeholders + // to dependencies, as well as create a public id for the condition. const condition = String(_condition); const [key, ifTrueDep, ifFalseDep] = condition.split(':'); const condHash = hashString(condition); diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index d6905d22543..3890da4833b 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -328,25 +328,25 @@ export default class BundleGraph ); } - getConditionPublicId(condition: string): string { + unstable_getConditionPublicId(condition: string): ?string { if (!getFeatureFlag('conditionalBundling')) { throw new Error( - 'getConditionPublicId called but conditionalBundling is not enabled', + 'unstable_getConditionPublicId called but conditionalBundling is not enabled', ); } - if (!this.#graph._conditions.has(condition)) { - throw new Error(`Condition ${condition} was not in mapping`); - } - return this.#graph._conditions.get(condition)?.publicId ?? ''; + return this.#graph._conditions.get(condition)?.publicId ?? null; } - // FIXME improve these lookups + // Given a set of dependencies, return any conditions where those dependencies are either + // the true or false dependency for those conditions. This is currently used to work out which + // conditions belong to a bundle in packaging. getConditionsForDependencies(deps: Array): Set<{| key: string, dependency: Dependency, ifTrue: string, ifFalse: string, |}> { + // FIXME improve these lookups const conditions = new Set(); const depIds = deps.map(dep => dep.id); for (const [, condition] of this.#graph._conditions.entries()) { @@ -375,10 +375,14 @@ export default class BundleGraph }); } } + // FIXME work out what's missing here.. (Flow) return conditions; } - getConditionalBundleMapping(): {| + // This is used to generate information for building a manifest that can + // be used by a webserver to understand which conditions are used by which bundles, + // and which bundles those conditions require depending on what they evaluate to. + unstable_getConditionalBundleMapping(): {| [string]: {| bundlesWithCondition: Array, ifTrueBundles: Array, @@ -419,35 +423,4 @@ export default class BundleGraph } return conditions; } - - getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}} { - if (!getFeatureFlag('conditionalBundling')) { - throw new Error( - 'getCondtionMapping called but conditionalBundling is not enabled', - ); - } - const ret = {}; - // console.log(this.#graph._conditions); - for (const [k, condition] of this.#graph._conditions.entries()) { - const [feature] = k.split(':'); - const [trueAsset, falseAsset] = [ - condition.ifTrueDependency, - condition.ifFalseDependency, - ].map(dep => { - const resolved = nullthrows(this.#graph.resolveAsyncDependency(dep)); - if (resolved.type === 'asset') { - return resolved.value; - } else { - return this.#graph.getAssetById(resolved.value.entryAssetId); - } - }); - - ret[condition.publicId] = { - ff: feature, - t: this.#graph.getAssetPublicId(trueAsset), - f: this.#graph.getAssetPublicId(falseAsset), - }; - } - return ret; - } } diff --git a/packages/core/core/src/public/MutableBundleGraph.js b/packages/core/core/src/public/MutableBundleGraph.js index 2679a994f42..ec8adcff5f0 100644 --- a/packages/core/core/src/public/MutableBundleGraph.js +++ b/packages/core/core/src/public/MutableBundleGraph.js @@ -39,7 +39,6 @@ export default class MutableBundleGraph #graph /*: InternalBundleGraph */; #options /*: ParcelOptions */; #bundlePublicIds /*: Set */ = new Set(); - #bundleConditions /* : Map */ = new Map(); constructor(graph: InternalBundleGraph, options: ParcelOptions) { super(graph, Bundle.get.bind(Bundle), options); @@ -52,40 +51,6 @@ export default class MutableBundleGraph assetToAssetValue(asset), bundleToInternalBundle(bundle), ); - - // TEST - // if (asset.meta.conditions != null && Array.isArray(asset.meta.conditions)) { - // for (const _condition of asset.meta.conditions ?? []) { - // const condition = String(_condition); - // if (!this.#bundleConditions.has(condition)) { - // const condHash = hashString(condition); - // const condPublicId = getPublicId(condHash, v => - // this.#bundleConditions.has(v), - // ); - - // const [, ifTrueDepPlaceholder, ifFalseDepPlaceholder] = condition.split(':'); - // let ifTrueDep, ifFalseDep; - // for (const dep of asset.getDependencies()) { - // if (dep.meta?.placeholder === ifTrueDepPlaceholder) { - // ifTrueDep = dependencyToInternalDependency(dep); - // } - // if (dep.meta?.placeholder === ifFalseDepPlaceholder) { - // ifFalseDep = dependencyToInternalDependency(dep); - // } - // } - // if (ifTrueDep == null || ifFalseDep == null) { - // throw new Error(`Deps were bad`); - // } - - // // FIXME is this the right way around?? It is for packaging.. - // this.#bundleConditions.set(condition, { - // publicId: condPublicId, - // ifTrueDependency: ifTrueDep, - // ifFalseDependency: ifFalseDep, - // }); - // } - // } - // } } addAssetGraphToBundle( @@ -353,8 +318,4 @@ export default class MutableBundleGraph bundleToInternalBundle(bundle), ); } - - getConditions(): Map { - return this.#bundleConditions; - } } diff --git a/packages/core/types-internal/src/index.js b/packages/core/types-internal/src/index.js index 5c4835849c7..99a11667ac5 100644 --- a/packages/core/types-internal/src/index.js +++ b/packages/core/types-internal/src/index.js @@ -1613,8 +1613,14 @@ export interface BundleGraph { getUsedSymbols(Asset | Dependency): ?$ReadOnlySet; /** Returns the common root directory for the entry assets of a target. */ getEntryRoot(target: Target): FilePath; - getConditionPublicId(condition: string): string; - getConditionMapping(): {[string]: {|ff: string, t: string, f: string|}}; + unstable_getConditionPublicId(condition: string): ?string; + unstable_getConditionalBundleMapping(): {| + [string]: {| + bundlesWithCondition: Array, + ifTrueBundles: Array, + ifFalseBundles: Array, + |}, + |}; } /** diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 3e22f64df2a..d8227ff726e 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -38,8 +38,11 @@ import {getFeatureFlag} from '@parcel/feature-flags'; // General regex used to replace imports with the resolved code, references with resolutions, // and count the number of newlines in the file for source maps. -const REPLACEMENT_RE = - /\n|import\s+"([0-9a-f]{16}:.+?)";|(?:\$[0-9a-f]{16}\$exports)|(?:\$[0-9a-f]{16}\$(?:import|importAsync|importCond|require)\$[0-9a-f]+(?:\$[0-9a-f]+)?)/g; +// +// For conditional bundling the only difference in this regex is adding `importCond` where we have `importAsync` etc.. +const REPLACEMENT_RE = getFeatureFlag('conditionalBundling') + ? /\n|import\s+"([0-9a-f]{16}:.+?)";|(?:\$[0-9a-f]{16}\$exports)|(?:\$[0-9a-f]{16}\$(?:import|importAsync|importCond|require)\$[0-9a-f]+(?:\$[0-9a-f]+)?)/g + : /\n|import\s+"([0-9a-f]{16}:.+?)";|(?:\$[0-9a-f]{16}\$exports)|(?:\$[0-9a-f]{16}\$(?:import|importAsync|require)\$[0-9a-f]+(?:\$[0-9a-f]+)?)/g; const BUILTINS = Object.keys(globals.builtin); const GLOBALS_BY_CONTEXT = { diff --git a/packages/packagers/js/src/dev-prelude.js b/packages/packagers/js/src/dev-prelude.js index 7e836cfca71..5636da413bd 100644 --- a/packages/packagers/js/src/dev-prelude.js +++ b/packages/packagers/js/src/dev-prelude.js @@ -109,14 +109,6 @@ {}, ]; }; - newRequire.conditions = previousRequire.conditions; - newRequire.registerConditions = function (conditions) { - for (var key in conditions) { - if (conditions.hasOwnProperty(key)) { - newRequire.conditions[key] = conditions[key]; - } - } - }; Object.defineProperty(newRequire, 'root', { get: function () { diff --git a/packages/packagers/js/src/utils.js b/packages/packagers/js/src/utils.js index f16fcc8d3cf..59e981f43eb 100644 --- a/packages/packagers/js/src/utils.js +++ b/packages/packagers/js/src/utils.js @@ -25,6 +25,7 @@ export function replaceScriptDependencies( lineCount++; return '\n'; } + let dep = nullthrows(dependencies.find(d => getSpecifier(d) === s)); let resolved = nullthrows(bundleGraph.getResolvedAsset(dep, bundle)); let publicId = bundleGraph.getAssetPublicId(resolved); diff --git a/packages/runtimes/js/src/JSRuntime.js b/packages/runtimes/js/src/JSRuntime.js index 14fad35cd5f..0bf0a59f9ae 100644 --- a/packages/runtimes/js/src/JSRuntime.js +++ b/packages/runtimes/js/src/JSRuntime.js @@ -190,6 +190,9 @@ export default (new Runtime({ } if (getFeatureFlag('conditionalBundling')) { + // For any conditions that are used in this bundle, we want to produce a runtime asset that is used to + // select the correct dependency that condition maps to at runtime - the conditions in the bundle will then be + // replaced with a reference to this asset to implement the selection. const conditions = bundleGraph.getConditionsForDependencies( conditionalDependencies, ); diff --git a/packages/runtimes/js/src/helpers/bundle-manifest.js b/packages/runtimes/js/src/helpers/bundle-manifest.js index 4b28d19bf4e..7aaa4dda570 100644 --- a/packages/runtimes/js/src/helpers/bundle-manifest.js +++ b/packages/runtimes/js/src/helpers/bundle-manifest.js @@ -1,5 +1,4 @@ var mapping = new Map(); -var conditions = new Map(); function register(baseUrl, manifest) { for (var i = 0; i < manifest.length - 1; i += 2) { mapping.set(manifest[i], { @@ -17,12 +16,5 @@ function resolve(id) { return new URL(resolved.path, resolved.baseUrl).toString(); } -function registerConditions(conditions) { - for (const [k, v] of Object.entries(conditions)) { - conditions.set(k, v); - } -} - module.exports.register = register; module.exports.resolve = resolve; -module.exports.registerConditions = registerConditions; From c87a1509a34648e6191c19256f7ef72d53fc78b0 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Fri, 19 Apr 2024 10:14:31 +1000 Subject: [PATCH 12/14] More comments --- packages/packagers/js/src/ScopeHoistingPackager.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index d8227ff726e..14d6378c013 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -1369,6 +1369,8 @@ ${code} } } // FIXME let's make all the bundles have the runtime with conditional bundling + // - we need to just make sure that _conditional bundles_ only have the runtime + // (as they likely get loaded before the entry bundle) if (this.needsPrelude || getFeatureFlag('conditionalBundling')) { // Add the prelude if this is potentially the first JS bundle to load in a // particular context (e.g. entry scripts in HTML, workers, etc.). From 606429519eee374397083ae4c2301d046d8bbfa6 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Fri, 19 Apr 2024 11:46:09 +1000 Subject: [PATCH 13/14] More documentation --- packages/transformers/js/core/src/dependency_collector.rs | 3 +++ packages/transformers/js/core/src/utils.rs | 2 ++ 2 files changed, 5 insertions(+) diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index b14361437ee..a7ba3c1dd36 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -712,6 +712,7 @@ impl<'a> Fold for DependencyCollector<'a> { rewrite_require_specifier(node, self.unresolved_mark) } else if self.config.conditional_bundling && kind == DependencyKind::ConditionalImport { let mut call = node; + // If we're not scope hositing, then change this `importCond` to a `require` if !self.config.scope_hoist { call.callee = ast::Callee::Expr(Box::new(ast::Expr::Ident(ast::Ident::new( "require".into(), @@ -724,6 +725,7 @@ impl<'a> Fold for DependencyCollector<'a> { panic!("importCond requires 3 arguments"); } let mut placeholders = Vec::new(); + // For the if_true and if_false arms of the conditional import, create a dependency for each arm for arg in &call.args[1..] { let specifier = match_str(&arg.expr).unwrap().0; let placeholder = self.add_dependency( @@ -742,6 +744,7 @@ impl<'a> Fold for DependencyCollector<'a> { placeholders.push(placeholder.unwrap()); } + // Create a condition we pass back to JS, which is of the form `key:if_true_placeholder:if_false_placeholder` let condition: JsWord = format!( "{}:{}:{}", match_str(&call.args[0].expr).unwrap().0, diff --git a/packages/transformers/js/core/src/utils.rs b/packages/transformers/js/core/src/utils.rs index 995e4fd14e0..156da40296a 100644 --- a/packages/transformers/js/core/src/utils.rs +++ b/packages/transformers/js/core/src/utils.rs @@ -156,6 +156,8 @@ pub fn match_require(node: &ast::Expr, unresolved_mark: Mark, ignore_mark: Mark) } } +/// This matches an expression like `importCond('if_true_dependency_id`, 'if_false_dependency_id')` and +/// returns the two dependency ids. pub fn match_import_cond(node: &ast::Expr, ignore_mark: Mark) -> Option<(JsWord, JsWord)> { use ast::*; From 61beb2fd9d5dc7e22d954ed7fe2a61ffdc5b2b24 Mon Sep 17 00:00:00 2001 From: Marcin Szczepanski Date: Mon, 22 Apr 2024 10:06:02 +1000 Subject: [PATCH 14/14] Cleanup some unused code, use condition object not string --- packages/core/core/src/BundleGraph.js | 35 +++++++++++++------ packages/core/core/src/public/BundleGraph.js | 11 +----- .../js/core/src/dependency_collector.rs | 25 +++++++------ packages/transformers/js/core/src/lib.rs | 2 +- packages/transformers/js/src/JSTransformer.js | 6 +++- 5 files changed, 46 insertions(+), 33 deletions(-) diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 4856b90b245..19687b5d553 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -139,7 +139,7 @@ export default class BundleGraph { /** The internal core Graph structure */ _graph: ContentGraph; _bundlePublicIds /*: Set */ = new Set(); - _conditions /*: Map */ = new Map(); + _conditions /*: Set */ = new Set(); constructor({ graph, @@ -152,7 +152,7 @@ export default class BundleGraph { publicIdByAssetId: Map, assetPublicIds: Set, bundleContentHashes: Map, - conditions: Map, + conditions: Set, |}) { this._graph = graph; this._assetPublicIds = assetPublicIds; @@ -225,13 +225,25 @@ export default class BundleGraph { if (getFeatureFlag('conditionalBundling') && node.type === 'asset') { const asset = node.value; if (Array.isArray(asset.meta.conditions)) { - for (const _condition of asset.meta.conditions ?? []) { - // Asset meta conditions will be of the form `key:placeholder_if_true:placeholfder_if_false` - // Here we extract that information and create `Condition`s which resolve those placeholders - // to dependencies, as well as create a public id for the condition. - const condition = String(_condition); - const [key, ifTrueDep, ifFalseDep] = condition.split(':'); - const condHash = hashString(condition); + for (const condition of asset.meta.conditions ?? []) { + // Resolve the placeholders that were attached to the asset in JSTransformer to dependencies, + // as well as create a public id for the condition. + + // $FlowFixMe[incompatible-type] + const { + key, + ifTruePlaceholder, + ifFalsePlaceholder, + }: { + key: string, + ifTruePlaceholder: string, + ifFalsePlaceholder: string, + ... + } = condition; + + const condHash = hashString( + `${key}:${ifTruePlaceholder}:${ifFalsePlaceholder}`, + ); const condPublicId = getPublicId(condHash, v => conditions.has(v)); conditions.set(condition, { @@ -239,8 +251,9 @@ export default class BundleGraph { // FIXME support the same condition used across multiple assets.. assets: new Set([asset]), key, - ifTrueDependency: placeholderToDependency.get(ifTrueDep), - ifFalseDependency: placeholderToDependency.get(ifFalseDep), + ifTrueDependency: placeholderToDependency.get(ifTruePlaceholder), + ifFalseDependency: + placeholderToDependency.get(ifFalsePlaceholder), }); } } diff --git a/packages/core/core/src/public/BundleGraph.js b/packages/core/core/src/public/BundleGraph.js index 3890da4833b..b7bdf063dd1 100644 --- a/packages/core/core/src/public/BundleGraph.js +++ b/packages/core/core/src/public/BundleGraph.js @@ -328,15 +328,6 @@ export default class BundleGraph ); } - unstable_getConditionPublicId(condition: string): ?string { - if (!getFeatureFlag('conditionalBundling')) { - throw new Error( - 'unstable_getConditionPublicId called but conditionalBundling is not enabled', - ); - } - return this.#graph._conditions.get(condition)?.publicId ?? null; - } - // Given a set of dependencies, return any conditions where those dependencies are either // the true or false dependency for those conditions. This is currently used to work out which // conditions belong to a bundle in packaging. @@ -349,7 +340,7 @@ export default class BundleGraph // FIXME improve these lookups const conditions = new Set(); const depIds = deps.map(dep => dep.id); - for (const [, condition] of this.#graph._conditions.entries()) { + for (const condition of this.#graph._conditions.values()) { if ( depIds.includes(condition.ifTrueDependency.id) || depIds.includes(condition.ifFalseDependency.id) diff --git a/packages/transformers/js/core/src/dependency_collector.rs b/packages/transformers/js/core/src/dependency_collector.rs index a7ba3c1dd36..ec288234350 100644 --- a/packages/transformers/js/core/src/dependency_collector.rs +++ b/packages/transformers/js/core/src/dependency_collector.rs @@ -56,6 +56,13 @@ pub struct DependencyDescriptor { pub placeholder: Option, } +#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] +pub struct Condition { + pub key: JsWord, + pub if_true_placeholder: Option, + pub if_false_placeholder: Option, +} + /// This pass collects dependencies in a module and compiles references as needed to work with Parcel's JSRuntime. pub fn dependency_collector<'a>( source_map: &'a SourceMap, @@ -64,7 +71,7 @@ pub fn dependency_collector<'a>( unresolved_mark: swc_core::common::Mark, config: &'a Config, diagnostics: &'a mut Vec, - conditions: &'a mut HashSet, + conditions: &'a mut HashSet, ) -> impl Fold + 'a { DependencyCollector { source_map, @@ -92,7 +99,7 @@ struct DependencyCollector<'a> { config: &'a Config, diagnostics: &'a mut Vec, import_meta: Option, - conditions: &'a mut HashSet, + conditions: &'a mut HashSet, } impl<'a> DependencyCollector<'a> { @@ -744,14 +751,12 @@ impl<'a> Fold for DependencyCollector<'a> { placeholders.push(placeholder.unwrap()); } - // Create a condition we pass back to JS, which is of the form `key:if_true_placeholder:if_false_placeholder` - let condition: JsWord = format!( - "{}:{}:{}", - match_str(&call.args[0].expr).unwrap().0, - placeholders[0], - placeholders[1] - ) - .into(); + // Create a condition we pass back to JS + let condition = Condition { + key: match_str(&call.args[0].expr).unwrap().0, + if_true_placeholder: Some(placeholders[0].clone()), + if_false_placeholder: Some(placeholders[1].clone()), + }; self.conditions.insert(condition); // write out code like importCond(depIfTrue, depIfFalse) - while we use the first dep as the link to the conditions diff --git a/packages/transformers/js/core/src/lib.rs b/packages/transformers/js/core/src/lib.rs index 38ed655c1b6..75fe8b10957 100644 --- a/packages/transformers/js/core/src/lib.rs +++ b/packages/transformers/js/core/src/lib.rs @@ -110,7 +110,7 @@ pub struct TransformResult { used_env: HashSet, has_node_replacements: bool, is_constant_module: bool, - conditions: HashSet, + conditions: HashSet, } fn targets_to_versions(targets: &Option>) -> Option { diff --git a/packages/transformers/js/src/JSTransformer.js b/packages/transformers/js/src/JSTransformer.js index 306fb77e097..9df7b842aae 100644 --- a/packages/transformers/js/src/JSTransformer.js +++ b/packages/transformers/js/src/JSTransformer.js @@ -578,7 +578,11 @@ export default (new Transformer({ : null, }); - asset.meta.conditions = conditions; + asset.meta.conditions = conditions.map(c => ({ + key: c.key, + ifTruePlaceholder: c.if_true_placeholder, + ifFalsePlaceholder: c.if_false_placeholder, + })); if (is_constant_module) { asset.meta.isConstantModule = true;