Skip to content

Commit

Permalink
Fix: Insert stylesheets that previously suspended
Browse files Browse the repository at this point in the history
Follow up to facebook#26450. In `acquireResource`, if a stylesheet resource
already has an instance, we need to confirm that it was inserted into
the document, because it may have been suspended. This happens because
stylesheet resources are assigned a resource before committing, inside
`suspendResource`. Probably a factoring a smell.

As currently implemented, the idea is that `suspendResource` does all
the same stuff as `acquireResource` except for the insertion.
  • Loading branch information
acdlite committed Mar 26, 2023
1 parent d12bdcd commit 8d5047e
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
33 changes: 31 additions & 2 deletions packages/react-dom-bindings/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -1788,7 +1788,7 @@ type StyleTagResource = TResource<'style', null>;
type StyleResource = StyleTagResource | StylesheetResource;
type ScriptResource = TResource<'script', null>;
type VoidResource = TResource<'void', null>;
type Resource = StyleResource | ScriptResource | VoidResource;
export type Resource = StyleResource | ScriptResource | VoidResource;

type LoadingState = number;
const NotLoaded = /* */ 0b000;
Expand Down Expand Up @@ -2170,6 +2170,7 @@ function preinit(href: string, options: PreinitOptions) {
state.loading |= Errored;
});

state.loading |= Inserted;
insertStylesheet(instance, precedence, resourceRoot);
}

Expand Down Expand Up @@ -2518,6 +2519,11 @@ export function acquireResource(

markNodeAsHoistable(instance);
setInitialProperties(instance, 'style', styleProps);

// TODO: `style` does not have loading state for tracking insertions. I
// guess because these aren't suspensey? Not sure whether this is a
// factoring smell.
// resource.state.loading |= Inserted;
insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot);
resource.instance = instance;

Expand Down Expand Up @@ -2556,6 +2562,7 @@ export function acquireResource(
linkInstance.onerror = reject;
});
setInitialProperties(instance, 'link', stylesheetProps);
resource.state.loading |= Inserted;
insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot);
resource.instance = instance;

Expand Down Expand Up @@ -2604,6 +2611,28 @@ export function acquireResource(
);
}
}
} else {
// In the case of stylesheets, they might have already been assigned an
// instance during `suspendResource`. But that doesn't mean they were
// inserted, because the commit might have been interrupted. So we need to
// check now.
//
// The other resource types are unaffected because they are not
// yet suspensey.
//
// TODO: This is a bit of a code smell. Consider refactoring how
// `suspendResource` and `acquireResource` work together. The idea is that
// `suspendResource` does all the same stuff as `acquireResource` except
// for the insertion.
if (
resource.type === 'stylesheet' &&
(resource.state.loading & Inserted) === NotLoaded
) {
const qualifiedProps: StylesheetQualifyingProps = props;
const instance: Instance = resource.instance;
resource.state.loading |= Inserted;
insertStylesheet(instance, qualifiedProps.precedence, hoistableRoot);
}
}
return resource.instance;
}
Expand All @@ -2613,7 +2642,7 @@ export function releaseResource(resource: Resource): void {
}

function insertStylesheet(
instance: HTMLElement,
instance: Element,
precedence: string,
root: HoistableRoot,
): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ export const errorHydratingContainer = $$$hostConfig.errorHydratingContainer;
// (optional)
// -------------------
export type HoistableRoot = mixed;
export type Resource = mixed; // eslint-disable-line no-undef
export const supportsResources = $$$hostConfig.supportsResources;
export const isHostHoistableType = $$$hostConfig.isHostHoistableType;
export const getHoistableRoot = $$$hostConfig.getHoistableRoot;
Expand Down

0 comments on commit 8d5047e

Please sign in to comment.