Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(turbopack): Fix NextEcmascriptClientReferenceTransition #70603

Merged
merged 5 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl EcmascriptClientReferenceProxyModule {
async fn proxy_module(&self) -> Result<Vc<Box<dyn EcmascriptChunkPlaceable>>> {
let mut code = CodeBuilder::default();

let server_module_path = &*self.server_module_ident.path().to_string().await?;
let server_module_path = &*self.server_module_ident.to_string().await?;

// Adapted from https://github.com/facebook/react/blob/c5b9375767e2c4102d7e5559d383523736f1c902/packages/react-server-dom-webpack/src/ReactFlightWebpackNodeLoader.js#L323-L354
if let EcmascriptExports::EsmExports(exports) = &*self.client_module.get_exports().await? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use turbopack::{
use turbopack_core::{
context::ProcessResult,
file_source::FileSource,
reference_type::{EntryReferenceSubType, ReferenceType},
reference_type::{EcmaScriptModulesReferenceSubType, EntryReferenceSubType, ReferenceType},
source::Source,
};
use turbopack_ecmascript::chunk::EcmascriptChunkPlaceable;
Expand Down Expand Up @@ -47,46 +47,58 @@ impl Transition for NextEcmascriptClientReferenceTransition {
self: Vc<Self>,
source: Vc<Box<dyn Source>>,
module_asset_context: Vc<ModuleAssetContext>,
_reference_type: Value<ReferenceType>,
reference_type: Value<ReferenceType>,
) -> Result<Vc<ProcessResult>> {
let part = match &*reference_type {
ReferenceType::EcmaScriptModules(EcmaScriptModulesReferenceSubType::ImportPart(
part,
)) => Some(*part),
_ => None,
};

let module_asset_context = self.process_context(module_asset_context);

let this = self.await?;

let ident = source.ident().await?;
let ident_path = ident.path.await?;
let ident = match part {
Some(part) => source.ident().with_part(part),
None => source.ident(),
};
let ident_ref = ident.await?;
let ident_path = ident_ref.path.await?;
let client_source = if ident_path.path.contains("next/dist/esm/") {
let path = ident.path.root().join(
let path = ident_ref.path.root().join(
ident_path
.path
.replace("next/dist/esm/", "next/dist/")
.into(),
);
Vc::upcast(FileSource::new_with_query(path, ident.query))
Vc::upcast(FileSource::new_with_query(path, ident_ref.query))
} else {
source
};
let client_module = this
.client_transition
.process(
client_source,
module_asset_context,
Value::new(ReferenceType::Entry(
EntryReferenceSubType::AppClientComponent,
)),
)
.module();
let client_module = this.client_transition.process(
client_source,
module_asset_context,
Value::new(ReferenceType::Entry(
Copy link
Member

Choose a reason for hiding this comment

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

That won't work if you don't pass part here, does it? Otherwise is just references the full module

EntryReferenceSubType::AppClientComponent,
)),
);
let ProcessResult::Module(client_module) = *client_module.await? else {
return Ok(ProcessResult::Ignore.cell());
};

let ssr_module = this.ssr_transition.process(
source,
module_asset_context,
Value::new(ReferenceType::Entry(
Copy link
Member

Choose a reason for hiding this comment

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

Same

EntryReferenceSubType::AppClientComponent,
)),
);

let ssr_module = this
.ssr_transition
.process(
source,
module_asset_context,
Value::new(ReferenceType::Entry(
EntryReferenceSubType::AppClientComponent,
)),
)
.module();
let ProcessResult::Module(ssr_module) = *ssr_module.await? else {
return Ok(ProcessResult::Ignore.cell());
};

let Some(client_module) =
Vc::try_resolve_sidecast::<Box<dyn EcmascriptChunkPlaceable>>(client_module).await?
Expand All @@ -113,7 +125,7 @@ impl Transition for NextEcmascriptClientReferenceTransition {

Ok(
ProcessResult::Module(Vc::upcast(EcmascriptClientReferenceProxyModule::new(
source.ident(),
ident,
Vc::upcast(server_context),
client_module,
ssr_module,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,7 @@ impl ClientReferenceManifest {
{
let ecmascript_client_reference = ecmascript_client_reference.await?;

let server_path = ecmascript_client_reference
.server_ident
.path()
.to_string()
.await?;
let server_path = ecmascript_client_reference.server_ident.to_string().await?;

let client_chunk_item = ecmascript_client_reference
.client_module
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,41 +31,40 @@ describe('app-dir - client-actions-tree-shaking', () => {

it('should not bundle unused server reference id in client bundles', async () => {
const appDir = next.testDir
const route1Files = await fs.readdir(
join(appDir, '.next/static/chunks/app/route-1')
)
const route2Files = await fs.readdir(
join(appDir, '.next/static/chunks/app/route-2')
)
const route3Files = await fs.readdir(
join(appDir, '.next/static/chunks/app/route-3')

const appBuildManifest = require(
join(appDir, '.next/app-build-manifest.json')
)

const route1Bundle = await fs.readFile(
join(
appDir,
'.next/static/chunks/app/route-1',
route1Files.find((file) => file.endsWith('.js'))
const bundle1Files = appBuildManifest.pages['/route-1/page']
const bundle2Files = appBuildManifest.pages['/route-2/page']
const bundle3Files = appBuildManifest.pages['/route-3/page']

const bundle1Contents = await Promise.all(
bundle1Files.map((file: string) =>
fs.readFile(join(appDir, '.next', file), 'utf8')
)
)
const route2Bundle = await fs.readFile(
join(
appDir,
'.next/static/chunks/app/route-2',
route2Files.find((file) => file.endsWith('.js'))
const bundle2Contents = await Promise.all(
bundle2Files.map((file: string) =>
fs.readFile(join(appDir, '.next', file), 'utf8')
)
)
const route3Bundle = await fs.readFile(
join(
appDir,
'.next/static/chunks/app/route-3',
route3Files.find((file) => file.endsWith('.js'))
const bundle3Contents = await Promise.all(
bundle3Files.map((file: string) =>
fs.readFile(join(appDir, '.next', file), 'utf8')
)
)

const bundle1Ids = getServerReferenceIdsFromBundle(route1Bundle.toString())
const bundle2Ids = getServerReferenceIdsFromBundle(route2Bundle.toString())
const bundle3Ids = getServerReferenceIdsFromBundle(route3Bundle.toString())
const bundle1Ids = bundle1Contents.flatMap((file: string) =>
getServerReferenceIdsFromBundle(file)
)
const bundle2Ids = bundle2Contents.flatMap((file: string) =>
getServerReferenceIdsFromBundle(file)
)
const bundle3Ids = bundle3Contents.flatMap((file: string) =>
getServerReferenceIdsFromBundle(file)
)

// Bundle 1 and 2 should only have one ID.
expect(bundle1Ids).toHaveLength(1)
Expand Down
4 changes: 2 additions & 2 deletions test/turbopack-build-tests-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -15694,10 +15694,10 @@
"runtimeError": false
},
"test/production/app-dir/actions-tree-shaking/client-actions-tree-shaking/client-actions-tree-shaking.test.ts": {
"passed": [],
"failed": [
"passed": [
"app-dir - client-actions-tree-shaking should not bundle unused server reference id in client bundles"
],
"failed": [],
"pending": [],
"flakey": [],
"runtimeError": false
Expand Down
Loading