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.

Sign up for GitHub

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

[Float] handle resource Resource creation inside svg context #25599

Merged
merged 3 commits into from
Nov 1, 2022
Merged

[Float] handle resource Resource creation inside svg context #25599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f83294b
do not make resources inside svg context
gnoff Nov 1, 2022
64f470a
only restrict title in svg. add warnings for almost-resoures in svg
gnoff Nov 1, 2022
6e949de
assert that foreign object allows resources
gnoff Nov 1, 2022
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
100 changes: 81 additions & 19 deletions packages/react-dom-bindings/src/client/ReactDOMFloatClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
getHostContext,
} from 'react-reconciler/src/ReactFiberHostContext';
import {getResourceFormOnly} from './validateDOMNesting';
import {getNamespace} from './ReactDOMHostConfig';
import {SVG_NAMESPACE} from '../shared/DOMNamespaces';

// The resource types we support. currently they match the form for the as argument.
// In the future this may need to change, especially when modules / scripts are supported
Expand Down Expand Up @@ -201,6 +203,28 @@ function getCurrentResourceRoot(): null | FloatRoot {
return currentContainer ? currentContainer.getRootNode() : null;
}

// This resource type constraint can be loosened. It really is everything except PreloadResource
// because that is the only one that does not have an optional instance type. Expand as needed.
function resetInstance(resource: ScriptResource | HeadResource) {
resource.instance = undefined;
}

export function clearRootResources(rootContainer: Container): void {
const rootNode: FloatRoot = (rootContainer.getRootNode(): any);
const resources = getResourcesFromRoot(rootNode);

// We can't actually delete the resource cache because this function is called
// during commit after we have rendered. Instead we detatch any instances from
// the Resource object if they are going to be cleared

// Styles stay put
// Scripts get reset
resources.scripts.forEach(resetInstance);
// Head Resources get reset
resources.head.forEach(resetInstance);
// lastStructuredMeta stays put
}

// Preloads are somewhat special. Even if we don't have the Document
// used by the root that is rendering a component trying to insert a preload
// we can still seed the file cache by doing the preload on any document we have
Expand Down Expand Up @@ -1077,7 +1101,14 @@ function acquireHeadResource(resource: HeadResource): Instance {
props,
root,
);
insertResourceInstanceBefore(root, instance, titles.item(0));
const firstTitle = titles[0];
insertResourceInstanceBefore(
root,
instance,
firstTitle && firstTitle.namespaceURI !== SVG_NAMESPACE
? firstTitle
: null,
);
break;
}
case 'meta': {
Expand Down Expand Up @@ -1397,16 +1428,21 @@ function insertResourceInstanceBefore(

export function isHostResourceType(type: string, props: Props): boolean {
let resourceFormOnly: boolean;
let namespace: string;
if (__DEV__) {
const hostContext = getHostContext();
resourceFormOnly = getResourceFormOnly(hostContext);
namespace = getNamespace(hostContext);
}
switch (type) {
case 'base':
case 'meta':
case 'title': {
case 'meta': {
return true;
}
case 'title': {
const hostContext = getHostContext();
return getNamespace(hostContext) !== SVG_NAMESPACE;
}
case 'link': {
const {onLoad, onError} = props;
if (onLoad || onError) {
Expand All @@ -1417,6 +1453,11 @@ export function isHostResourceType(type: string, props: Props): boolean {
' Try removing onLoad={...} and onError={...} or moving it into the root <head> tag or' +
' somewhere in the <body>.',
);
} else if (namespace === SVG_NAMESPACE) {
console.error(
'Cannot render a <link> with onLoad or onError listeners as a descendent of <svg>.' +
' Try removing onLoad={...} and onError={...} or moving it above the <svg> ancestor.',
);
}
}
return false;
Expand All @@ -1426,11 +1467,18 @@ export function isHostResourceType(type: string, props: Props): boolean {
const {href, precedence, disabled} = props;
if (__DEV__) {
validateLinkPropsForStyleResource(props);
if (typeof precedence !== 'string' && resourceFormOnly) {
console.error(
'Cannot render a <link rel="stylesheet" /> outside the main document without knowing its precedence.' +
' Consider adding precedence="default" or moving it into the root <head> tag.',
);
if (typeof precedence !== 'string') {
if (resourceFormOnly) {
console.error(
'Cannot render a <link rel="stylesheet" /> outside the main document without knowing its precedence.' +
' Consider adding precedence="default" or moving it into the root <head> tag.',
);
} else if (namespace === SVG_NAMESPACE) {
console.error(
'Cannot render a <link rel="stylesheet" /> as a descendent of an <svg> element without knowing its precedence.' +
' Consider adding precedence="default" or moving it above the <svg> ancestor.',
);
}
}
}
return (
Expand All @@ -1450,17 +1498,31 @@ export function isHostResourceType(type: string, props: Props): boolean {
// precedence with these for style resources
const {src, async, onLoad, onError} = props;
if (__DEV__) {
if (async !== true && resourceFormOnly) {
console.error(
'Cannot render a sync or defer <script> outside the main document without knowing its order.' +
' Try adding async="" or moving it into the root <head> tag.',
);
} else if ((onLoad || onError) && resourceFormOnly) {
console.error(
'Cannot render a <script> with onLoad or onError listeners outside the main document.' +
' Try removing onLoad={...} and onError={...} or moving it into the root <head> tag or' +
' somewhere in the <body>.',
);
if (async !== true) {
if (resourceFormOnly) {
console.error(
'Cannot render a sync or defer <script> outside the main document without knowing its order.' +
' Try adding async="" or moving it into the root <head> tag.',
);
} else if (namespace === SVG_NAMESPACE) {
console.error(
'Cannot render a sync or defer <script> as a descendent of an <svg> element.' +
' Try adding async="" or moving it above the ancestor <svg> element.',
);
}
} else if (onLoad || onError) {
if (resourceFormOnly) {
console.error(
'Cannot render a <script> with onLoad or onError listeners outside the main document.' +
' Try removing onLoad={...} and onError={...} or moving it into the root <head> tag or' +
' somewhere in the <body>.',
);
} else if (namespace === SVG_NAMESPACE) {
console.error(
'Cannot render a <script> with onLoad or onError listeners as a descendent of an <svg> element.' +
' Try removing onLoad={...} and onError={...} or moving it above the ancestor <svg> element.',
);
}
}
}
return (async: any) && typeof src === 'string' && !onLoad && !onError;
Expand Down
33 changes: 32 additions & 1 deletion packages/react-dom-bindings/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ import {ConcurrentMode, NoMode} from 'react-reconciler/src/ReactTypeOfMode';
import {
prepareToRenderResources,
cleanupAfterRenderResources,
clearRootResources,
isHostResourceType,
} from './ReactDOMFloatClient';

Expand Down Expand Up @@ -219,6 +220,16 @@ export function getPublicInstance(instance: Instance): Instance {
return instance;
}

export function getNamespace(hostContext: HostContext): string {
if (__DEV__) {
const hostContextDev: HostContextDev = (hostContext: any);
return hostContextDev.namespace;
} else {
const hostContextProd: HostContextProd = (hostContext: any);
return hostContextProd;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the other helpers that call out to external things in ReactDOMComponent read the namespace first and then pass it in as an argument to them.

I think we need to rip out getHostContext call and put those in the reconciler and then call the host config with the host context like we do for createInstance and others.

It's too fragile. I don't actually know if it's even correct to call it in the child reconciler. I think that might actually be wrong in some cases because I'm not sure that it's guaranteed to have the right context. Regardless, that code in the reconciler is what needs to provide that guarantee.

Once you pass it in to isHostResourceType, then you can get the namespace in isHostResourceType and pass it into ReactDOMFloatClient. It's free in prod since it's just the same value and hopefully the whole ReactDOMFloatClient gets inlined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is unforutnately buried pretty deep in the reconciliation pass. I'd have to thread it through reconcileChildren. I can do this but before I go and make that happen I want to make sure that's actually viable.

An alternative is we let the reconciliation create HostComponents always and then we "upgrade" them to Resource when we beginWork and currrent === null similar to how Memo's can become SimpleMemos

Copy link
Collaborator

@sebmarkbage sebmarkbage Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You can start by just calling it right before calling isHostResourceType. At least then it's in the reconciler code instead of the bindings. It's a pretty cheap call too that could possibly even be reorganized by the compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or it could be cheap #25609

}
}

gnoff marked this conversation as resolved.
Show resolved Hide resolved
export function prepareForCommit(containerInfo: Container): Object | null {
eventsEnabled = ReactBrowserEventEmitterIsEnabled();
selectionInformation = getSelectionInformation();
Expand Down Expand Up @@ -715,11 +726,18 @@ export function clearContainer(container: Container): void {
if (enableHostSingletons) {
const nodeType = container.nodeType;
if (nodeType === DOCUMENT_NODE) {
clearRootResources(container);
clearContainerSparingly(container);
} else if (nodeType === ELEMENT_NODE) {
switch (container.nodeName) {
case 'HEAD': {
// If we are clearing document.head as a container we are essentially clearing everything
// that was hoisted to the head and should forget the instances that will no longer be in the DOM
clearRootResources(container);
// fall through to clear child contents
}
// eslint-disable-next-line-no-fallthrough
case 'HTML':
case 'HEAD':
case 'BODY':
clearContainerSparingly(container);
return;
Expand Down Expand Up @@ -910,6 +928,19 @@ function getNextHydratable(node) {
if (nodeType === ELEMENT_NODE) {
const element: Element = (node: any);
switch (element.tagName) {
// This is subtle. in SVG scope the title tag is case sensitive. we don't want to skip
// titles in svg but we do want to skip them outside of svg. there is an edge case where
// you could do `React.createElement('TITLE', ...)` inside an svg scope but the SSR serializer
// will still emit lowercase. Practically speaking the only time the DOM will have a non-uppercased
// title tagName is if it is inside an svg.
// Other Resource types like META, BASE, LINK, and SCRIPT should be treated as resources even inside
// svg scope because they are invalid otherwise. We still don't need to handle the lowercase variant
// because if they are present in the DOM already they would have been hoisted outside the SVG scope
// as Resources. So while it would be correct to skip a <link> inside <svg> and this algorithm won't
// skip that link because the tagName will not be uppercased it functionally is irrelevant. If one
// tries to render incompatible types such as a non-resource stylesheet inside an svg the server will
// emit that invalid html and hydration will fail. In Dev this will present warnings guiding the
// developer on how to fix.
Comment on lines +931 to +943
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

please check my rationale here

case 'TITLE':
case 'META':
case 'BASE':
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,7 @@ function pushTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): ReactNodeList {
if (__DEV__) {
Expand Down Expand Up @@ -1415,6 +1416,8 @@ function pushTitle(

if (
enableFloat &&
// title is valid in SVG so we avoid resour
insertionMode !== SVG_MODE &&
!noscriptTagInScope &&
resourcesFromElement('title', props)
) {
Expand Down Expand Up @@ -1926,6 +1929,7 @@ export function pushStartInstance(
target,
props,
responseState,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
)
: pushStartTitle(target, props, responseState);
Expand Down
Loading