Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Outdated
Show resolved
Hide resolved
89cc65e to
24998d4
Compare
Merging this PR will degrade performance by 8.56%
Performance Changes
Comparing Footnotes
|
Stats from current PR✅ No significant changes detected📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles: **401 kB** → **401 kB** ✅ -14 B80 files with content-based hashes (individual files not comparable between builds) Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📎 Tarball URL |
24998d4 to
c23f620
Compare
This comment was marked as spam.
This comment was marked as spam.
Failing test suitesCommit: 6887358 | About building and testing Next.js
Expand output● app dir - navigation › middleware redirect › should change browser location when router.refresh() gets a redirect response |
2d09473 to
064c91d
Compare
064c91d to
4ad9df0
Compare
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Outdated
Show resolved
Hide resolved
06e4aff to
236f004
Compare
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Show resolved
Hide resolved
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Outdated
Show resolved
Hide resolved
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Outdated
Show resolved
Hide resolved
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Show resolved
Hide resolved
15c0416 to
2d7eb82
Compare
turbopack/crates/turbopack-ecmascript/src/references/constant_value.rs
Outdated
Show resolved
Hide resolved
turbopack/crates/turbopack-ecmascript/src/references/cross_module_constants.rs
Outdated
Show resolved
Hide resolved
| // TODO this is missing all source transforms (e.g. Webpakc loaders) | ||
| // | ||
| // TODO ideally we'd use `import_reference[i].resolve_reference().first().failsafe_parse()` for | ||
| // determining the constants. | ||
| // | ||
| // That slots nicely into the module system (custom modules could participate in this together | ||
| // with Webpack loaders), and you get side-effect-free barrel file optimization for free). The | ||
| // problem is that `resolve_reference`` creates all the tree shaking modules, for which we need | ||
| // the exports of the module, which analyzes the module, which potentially analyzes | ||
| // cross-module constants, leading to the execution cycle. | ||
| // | ||
| // That cycle would ideally to be broken somehow. | ||
| let source = esm_resolve_source( | ||
| origin, | ||
| Request::parse_string(module_value.module.to_string_lossy().into()), | ||
| EcmaScriptModulesReferenceSubType::Import, | ||
| ResolveErrorMode::Ignore, | ||
| None, | ||
| ) | ||
| .await? | ||
| .await?; | ||
|
|
||
| let Some(ResolveResultItem::Source(source)) = source.primary.first().as_ref().map(|v| &v.1) | ||
| else { | ||
| // failed to resolve, ignore silently | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| let constants = get_constants(**source, compile_time_info).await?; |
There was a problem hiding this comment.
This is very broken and a blocker for enabling this by default. This could break stuff when source transforms are involved. And I can cause weird parse errors when source files depend on source transforms to be parseable.
It also caused files to be double parsed.
We need to change that.
I think it should be possible to make process not call analyze of modules. We do call get_exports, but that can a lightweight pass over the parsed module and not a full analyze. That would also be beneficial for side effect free module optimization when barrel files can be skipped without a full analyze step on them.
| JsValue::Constant(ConstantValue::Str(key.clone().into())), | ||
| if let Some(value) = value { | ||
| if !has_opt_in { | ||
| // when not having opt in, only inline short literals | ||
| match value { | ||
| ConstantValue::Str(s) if s.as_str().len() > 6 => { | ||
| JsValue::unknown_empty(false, "constant too long") | ||
| } | ||
| ConstantValue::Num(n) if n.0.abs() > 1_000_000.0 => { | ||
| JsValue::unknown_empty(false, "constant too long") | ||
| } | ||
| ConstantValue::BigInt(n) | ||
| if **n > BigInt::from(1_000_000) | ||
| || **n < BigInt::from(-1_000_000) => | ||
| { | ||
| JsValue::unknown_empty(false, "constant too long") | ||
| } | ||
| ConstantValue::Regex(_) => { | ||
| // Regexes are literals, but they are also objects, so | ||
| // have identity and aren't inlined without opt in. | ||
| JsValue::unknown_empty(false, "regex not inlined") | ||
| } | ||
| _ => JsValue::Constant(value.clone()), | ||
| } | ||
| } else { | ||
| JsValue::Constant(value.clone()) | ||
| } | ||
| } else { | ||
| JsValue::unknown_empty(false, "not a constant") | ||
| }, |
There was a problem hiding this comment.
I would not expect this filtering to happen here.
I want to have compile-time values for everything,
but only inline values under certain conditions.
So I would expect the filtering happen in the inlining path.
There was a problem hiding this comment.
All of this would be much easier if the link step were actually intelligent. As opposed to doing only bottom-up replacements.
There was a problem hiding this comment.
But I think I can still achieve this somehow.
0cce7f6 to
6887358
Compare

This is now a proper compile-time constant:
You can use code to compute constants just fine, and use any existing constants such as
process.env.NODE_ENV.Currently, you can't use imports to other constants modules, but we can add that later on.
We can't perform this constants check for every single import, so you need to either
UPPER_CASEimport names as in the example aboveimport { lower } from './other' with { turbopackConstants: 'true' }Then that referenced module will be analyzed for constants, and if the referenced export is a constant, it will participate in constant inlining just as
process.env.FOO.This also works fine with barrel imports, you can still do
import { IS_DEV } from './barrel.js'and it will find theconstants.jsfile which in itself will indeed only have constants exports.Because it's not always opt-in at the import site, we can't automatically make non-constant exports an error. For that you have to add
'use turbopack constants'at the top of the module, which will make it an error if any constant import references that module, and the module has any non-constant exports:Some prior art: https://rspack.rs/blog/announcing-1-5#const-inline-optimization, https://rspack.rs/config/optimization#optimizationinlineexports
No compile-time impact on a big app: