Skip to content

Commit 74fd942

Browse files
authored
Fix bug with environment ID being ref
* Fix a missing ref conversion where the ref would be kept as an object even with the flag enabled * Fix rust compatibility with the environment deduplication flag A subsequent change will modify rust asset graph so it doesn't copy environments over. We expect a non-negligible performance improvement to native builds from that change. Test Plan: N/A Reviewers: mattcompiles, pancaspe87, marcins Reviewed By: mattcompiles, marcins, pancaspe87 Pull Request: #658
1 parent 5ded263 commit 74fd942

File tree

6 files changed

+30
-7
lines changed

6 files changed

+30
-7
lines changed

.changeset/stale-gorillas-teach.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@atlaspack/core': patch
3+
---
4+
5+
Fix environment deduplication issues

packages/core/core/src/AssetGraph.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,11 @@ import nullthrows from 'nullthrows';
2929
import {ContentGraph} from '@atlaspack/graph';
3030
import {createDependency} from './Dependency';
3131
import {type ProjectPath, fromProjectPathRelative} from './projectPath';
32-
import {fromEnvironmentId, toEnvironmentId} from './EnvironmentManager';
32+
import {
33+
fromEnvironmentId,
34+
toEnvironmentId,
35+
toEnvironmentRef,
36+
} from './EnvironmentManager';
3337
import {getFeatureFlag} from '@atlaspack/feature-flags';
3438

3539
type InitOpts = {|
@@ -160,7 +164,7 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
160164

161165
let env = this.envCache.get(idAndContext);
162166
if (env) {
163-
input.env = env;
167+
input.env = toEnvironmentRef(env);
164168
} else {
165169
this.envCache.set(idAndContext, fromEnvironmentId(input.env));
166170
}

packages/core/core/src/Environment.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ export function mergeEnvironments(
140140
});
141141
}
142142

143-
function getEnvironmentHash(env: Environment): string {
143+
export function getEnvironmentHash(env: Environment): string {
144144
const data = {
145145
context: env.context,
146146
engines: env.engines,

packages/core/core/src/EnvironmentManager.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export opaque type EnvironmentId = string;
2121
/**
2222
* When deduplication is cleaned-up this will always be a string.
2323
*/
24-
export type EnvironmentRef = EnvironmentId | CoreEnvironment;
24+
export opaque type EnvironmentRef = EnvironmentId | CoreEnvironment;
2525

2626
/**
2727
* Convert environment to a ref.

packages/core/core/src/public/MutableBundleGraph.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
} from '@atlaspack/types';
1212
import type {
1313
AtlaspackOptions,
14+
BundleGraphNode,
1415
BundleGroup as InternalBundleGroup,
1516
BundleNode,
1617
} from '../types';
@@ -207,7 +208,8 @@ export default class MutableBundleGraph
207208
bundleBehavior: opts.bundleBehavior ?? null,
208209
});
209210

210-
let existing = this.#graph._graph.getNodeByContentKey(bundleId);
211+
let existing: ?BundleGraphNode =
212+
this.#graph._graph.getNodeByContentKey(bundleId);
211213
if (existing != null) {
212214
invariant(existing.type === 'bundle');
213215
return Bundle.get(existing.value, this.#graph, this.#options);

packages/core/core/src/requests/AssetGraphRequestRust.js

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import invariant from 'assert';
55
import ThrowableDiagnostic from '@atlaspack/diagnostic';
66
import type {Async} from '@atlaspack/types';
77
import {instrument} from '@atlaspack/logger';
8+
import {getFeatureFlag} from '@atlaspack/feature-flags';
89

910
import AssetGraph, {nodeFromAssetGroup} from '../AssetGraph';
1011
import type {AtlaspackV3} from '../atlaspack-v3';
@@ -16,6 +17,8 @@ import type {
1617
AssetGraphRequestInput,
1718
AssetGraphRequestResult,
1819
} from './AssetGraphRequest';
20+
import {toEnvironmentRef} from '../EnvironmentManager';
21+
import {getEnvironmentHash} from '../Environment';
1922

2023
type RunInput = {|
2124
input: AssetGraphRequestInput,
@@ -166,9 +169,14 @@ export function getAssetGraph(serializedGraph: any): {
166169

167170
asset.committed = true;
168171
asset.contentKey = id;
169-
asset.env.id = getEnvId(asset.env);
172+
asset.env.id = getFeatureFlag('environmentDeduplication')
173+
? // TODO: Rust can do this and avoid copying a significant amount of data over
174+
getEnvironmentHash(asset.env)
175+
: getEnvId(asset.env);
170176
asset.mapKey = `map:${asset.id}`;
171177

178+
asset.env = toEnvironmentRef(asset.env);
179+
172180
// This is populated later when we map the edges between assets and dependencies
173181
asset.dependencies = new Map();
174182

@@ -195,7 +203,11 @@ export function getAssetGraph(serializedGraph: any): {
195203
let dependency = node.value.dependency;
196204

197205
dependency.id = id;
198-
dependency.env.id = getEnvId(dependency.env);
206+
dependency.env.id = getFeatureFlag('environmentDeduplication')
207+
? // TODO: Rust can do this and avoid copying a significant amount of data over
208+
getEnvironmentHash(dependency.env)
209+
: getEnvId(dependency.env);
210+
dependency.env = toEnvironmentRef(dependency.env);
199211

200212
if (dependency.symbols != null) {
201213
dependency.symbols = new Map(dependency.symbols?.map(mapSymbols));

0 commit comments

Comments
 (0)