Skip to content

Commit

Permalink
Fix hang up when loading duplicated CSS module chunks (vercel/turbore…
Browse files Browse the repository at this point in the history
…po#4879)

### Description

I identified this issue when authoring a test for PostCSS. Here's how it
goes:

1. CSS chunk 1 loads with module chunk A and source = runtime. This
marks chunk A as an available module chunk. The promise for A is the
same promise as for CSS chunk 1, which is resolved in registerChunk
because chunk 1 is part of the chunk group of the initial JS chunk.

2. CSS chunk 2 loads with module chunks A and B. Since A is already
available, we run into a different branch, where we only try to load B.
However, since we're still using source = runtime, we don't actually
attempt to load B and instead return a promise to the resolver of B. But
B isn't part of the chunk group of the initial JS chunk, so it is never
resolved.

This hangs up the runtime entirely and breaks page hydration.

Instead, we move the "automatically resolving CSS chunks when source =
Runtime" from `registerChunk` (which wasn't quite correct either as far
as I can tell) into `loadChunk`, which solves this issue.

### Testing Instructions

I don't know if it's worth having a test case just for this, but this
will technically be covered under the PostCSS test case in
#49463.
link WEB-1023
  • Loading branch information
alexkirsz authored May 9, 2023
1 parent 116aa6e commit d3d7f01
Show file tree
Hide file tree
Showing 36 changed files with 540 additions and 504 deletions.
29 changes: 15 additions & 14 deletions crates/turbopack-dev/js/src/runtime.dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -169,12 +162,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1532,15 +1532,8 @@ let BACKEND;

for (const otherChunkData of params.otherChunks) {
const otherChunkPath = getChunkPath(otherChunkData);
if (otherChunkPath.endsWith(".css")) {
// Mark all CSS chunks within the same chunk group as this chunk as loaded.
// They are just injected as <link> tag and have to way to communicate completion.
const cssResolver = getOrCreateResolver(otherChunkPath);
cssResolver.resolve();
} else if (otherChunkPath.endsWith(".js")) {
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}
// Chunk might have started loading, so we want to avoid triggering another load.
getOrCreateResolver(otherChunkPath);
}

// This waits for chunks to be loaded, but also marks included items as available.
Expand Down Expand Up @@ -1683,12 +1676,20 @@ let BACKEND;
return resolver.promise;
}

// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.
// However, we need to wait for them to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absense of
// `resolver.resolve()` in this branch.
if (source.type === SourceTypeRuntime) {
// We don't need to load chunks references from runtime code, as they're already
// present in the DOM.

if (chunkPath.endsWith(".css")) {
// CSS chunks do not register themselves, and as such must be marked as
// loaded instantly.
resolver.resolve();
}

// We need to wait for JS chunks to register themselves within `registerChunk`
// before we can start instantiating runtime modules, hence the absence of
// `resolver.resolve()` in this branch.

return resolver.promise;
}

Expand Down
Loading

0 comments on commit d3d7f01

Please sign in to comment.