Skip to content

Commit 54b8dca

Browse files
tyao1meta-codesync[bot]
authored andcommitted
Add shard_extra_artifacts feature flag to shard PHP text artifacts
Summary: Adds a `ProjectConfig::create_path_for_extra_artifact()` helper in `relay-config` and a `shard_extra_artifacts` `FeatureFlag` in the shared OSS `FeatureFlags`. The helper mirrors `create_path_for_artifact()` (honoring `shardOutput` / `shardStripRegex`) but is rooted at `extraArtifactsOutput`, with sharding gated by an explicit `should_shard` argument so callers can roll it out behind a flag. No OSS call sites use the flag today — internal PHP text-artifact generation is the only current consumer. --- Today, generated PHP text artifacts (`RelayOperation.php`, `RelayFragment.php`) are written to a flat directory under `extraArtifactsOutput`, ignoring the project's `shardOutput` / `shardStripRegex` configuration. Normal JS artifacts use `ProjectConfig::create_path_for_artifact()` which shards output into subdirectories mirroring the source file's relative path — but the PHP codepath bypasses this entirely with a simple `extra_artifacts_output.join(filename)`. This diff adds: - A new `ProjectConfig::create_path_for_extra_artifact()` helper in `relay-config` that mirrors the sharding logic of `create_path_for_artifact()` but rooted at `extra_artifacts_output`. It takes an explicit `should_shard: bool` so callers can gate the behavior during rollout. - A `shard_extra_artifacts` `FeatureFlag` in the OSS `FeatureFlags` struct (same pattern as `text_artifacts` — both control FB-only PHP generation but live in the shared config for consistency with the `"featureFlags"` JSON block). - Wiring in `text_artifacts_generators.rs` to call the new helper, checking `feature_flags.shard_extra_artifacts.is_enabled_for(source_name)`. - Simplification of the `FBProjectConfig` fallback construction in `main.rs` to use `..Default::default()` (prevents breakage when new fields are added). - 5 unit tests covering: sharded with regex, sharded without regex, unsharded, shard flag ignored when project doesn't shard, and panic on missing `extra_artifacts_output`. When the flag is disabled (default), behavior is identical to today. When enabled, PHP artifacts are placed in subdirectories matching the source file layout (e.g. `flib/__generated__/.../js/enterprise/ui/foo/BarRelayOperation.php` instead of flat `flib/__generated__/.../BarRelayOperation.php`). Note: the Hack preloader (`hack_query_preloader.rs`) has the same flat-path issue but is intentionally not changed in this diff — follow-up. Reviewed By: evanyeung Differential Revision: D105719732 fbshipit-source-id: 2cd22a6d1db50a9c3b833a0b1ed07eef43ccceef
1 parent 6ed5470 commit 54b8dca

2 files changed

Lines changed: 135 additions & 0 deletions

File tree

compiler/crates/common/src/feature_flags.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ pub struct FeatureFlags {
5454
#[serde(default)]
5555
pub text_artifacts: FeatureFlag,
5656

57+
/// Shard generated extra artifacts into subdirectories under
58+
/// `extraArtifactsOutput` that mirror the source file's relative path,
59+
/// honoring `shardOutput` / `shardStripRegex`.
60+
#[serde(default)]
61+
pub shard_extra_artifacts: FeatureFlag,
62+
5763
#[serde(default)]
5864
pub skip_printing_nulls: FeatureFlag,
5965

@@ -223,6 +229,7 @@ impl Default for FeatureFlags {
223229
no_inline: Default::default(),
224230
enable_3d_branch_arg_generation: Default::default(),
225231
text_artifacts: Default::default(),
232+
shard_extra_artifacts: Default::default(),
226233
skip_printing_nulls: Default::default(),
227234
compact_query_text: Default::default(),
228235
enable_resolver_normalization_ast: Default::default(),

compiler/crates/relay-config/src/project_config.rs

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,50 @@ impl ProjectConfig {
447447
}
448448
}
449449

450+
/// Gets the path for a generated *extra* artifact based on its
451+
/// originating source file's location and the project's configuration.
452+
///
453+
/// Mirrors [`Self::create_path_for_artifact`], except the output root is
454+
/// `extra_artifacts_output`. Sharding is opt-in via `should_shard` —
455+
/// callers gate this on a feature flag while the new behavior is rolled
456+
/// out, so that passing `should_shard = false` reproduces the legacy
457+
/// flat layout (`extra_artifacts_output.join(file_name)`).
458+
///
459+
/// When `should_shard` is true but the project does not set
460+
/// `shardOutput`, the call silently falls back to the flat layout. This
461+
/// is intentional: the feature flag may be enabled across many projects
462+
/// during rollout, but only those that already opt into `shardOutput`
463+
/// get the new sharded layout.
464+
///
465+
/// Panics if `extra_artifacts_output` is unset; only projects that
466+
/// configure `extraArtifactsOutput` produce extra artifacts, so reaching
467+
/// this with no output configured indicates an internal invariant
468+
/// violation rather than user misconfiguration.
469+
pub fn create_path_for_extra_artifact(
470+
&self,
471+
source_file: SourceLocationKey,
472+
artifact_file_name: String,
473+
should_shard: bool,
474+
) -> PathBuf {
475+
let output = self.extra_artifacts_output.as_ref().expect(
476+
"create_path_for_extra_artifact called without `extraArtifactsOutput` set on the project config",
477+
);
478+
if should_shard && self.shard_output {
479+
if let Some(ref regex) = self.shard_strip_regex {
480+
let full_source_path = regex.replace_all(source_file.path(), "");
481+
let mut path = output.join(full_source_path.to_string());
482+
// `full_source_path` still includes the source file name; pop it to get the directory.
483+
path.pop();
484+
path
485+
} else {
486+
output.join(source_file.get_dir())
487+
}
488+
.join(artifact_file_name)
489+
} else {
490+
output.join(artifact_file_name)
491+
}
492+
}
493+
450494
/// Generates a path for an artifact file based on a definition name and its location.
451495
pub fn artifact_path_for_definition(
452496
&self,
@@ -533,3 +577,87 @@ fn format_normalized_path(path: &Path) -> String {
533577
.to_string()
534578
.replace(MAIN_SEPARATOR, "/")
535579
}
580+
581+
#[cfg(test)]
582+
mod tests {
583+
use super::*;
584+
585+
fn project_with_extra(
586+
extra_artifacts_output: Option<&str>,
587+
shard_output: bool,
588+
shard_strip_regex: Option<&str>,
589+
) -> ProjectConfig {
590+
ProjectConfig {
591+
extra_artifacts_output: extra_artifacts_output.map(PathBuf::from),
592+
shard_output,
593+
shard_strip_regex: shard_strip_regex.map(|r| Regex::new(r).unwrap()),
594+
..Default::default()
595+
}
596+
}
597+
598+
#[test]
599+
fn extra_artifact_path_unsharded_keeps_legacy_flat_layout() {
600+
let config = project_with_extra(Some("out/extra"), true, Some("^src/"));
601+
let path = config.create_path_for_extra_artifact(
602+
SourceLocationKey::standalone("src/components/foo/Bar.js"),
603+
"BarOperation.graphql.js".to_owned(),
604+
false,
605+
);
606+
assert_eq!(
607+
path,
608+
PathBuf::from("out/extra/BarOperation.graphql.js"),
609+
"shard=false should produce the same flat path as the legacy call site"
610+
);
611+
}
612+
613+
#[test]
614+
fn extra_artifact_path_sharded_with_regex_mirrors_normal_artifact_layout() {
615+
let config = project_with_extra(Some("out/extra"), true, Some("^src/"));
616+
let path = config.create_path_for_extra_artifact(
617+
SourceLocationKey::standalone("src/components/foo/Bar.js"),
618+
"BarOperation.graphql.js".to_owned(),
619+
true,
620+
);
621+
assert_eq!(
622+
path,
623+
PathBuf::from("out/extra/components/foo/BarOperation.graphql.js"),
624+
"Source 'src/components/foo/Bar.js' with regex '^src/' should shard under 'components/foo/'"
625+
);
626+
}
627+
628+
#[test]
629+
fn extra_artifact_path_sharded_without_regex_uses_full_source_dir() {
630+
let config = project_with_extra(Some("out/extra"), true, None);
631+
let path = config.create_path_for_extra_artifact(
632+
SourceLocationKey::standalone("a/b/c/Bar.js"),
633+
"BarOperation.graphql.js".to_owned(),
634+
true,
635+
);
636+
assert_eq!(
637+
path,
638+
PathBuf::from("out/extra/a/b/c/BarOperation.graphql.js"),
639+
);
640+
}
641+
642+
#[test]
643+
fn extra_artifact_path_shard_arg_ignored_when_project_does_not_shard() {
644+
let config = project_with_extra(Some("out/extra"), false, Some("^src/"));
645+
let path = config.create_path_for_extra_artifact(
646+
SourceLocationKey::standalone("src/components/foo/Bar.js"),
647+
"BarOperation.graphql.js".to_owned(),
648+
true,
649+
);
650+
assert_eq!(path, PathBuf::from("out/extra/BarOperation.graphql.js"),);
651+
}
652+
653+
#[test]
654+
#[should_panic(expected = "extraArtifactsOutput")]
655+
fn extra_artifact_path_panics_without_extra_artifacts_output() {
656+
let config = project_with_extra(None, true, Some("^src/"));
657+
let _ = config.create_path_for_extra_artifact(
658+
SourceLocationKey::standalone("src/components/foo/Bar.js"),
659+
"Bar.graphql.js".to_owned(),
660+
true,
661+
);
662+
}
663+
}

0 commit comments

Comments
 (0)