Skip to content

[Float] add support for scripts and other enhancements #25480

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

Merged
merged 2 commits into from
Oct 17, 2022
+1,397 −495
Merged

[Float] add support for scripts and other enhancements #25480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6e93c7
float enhance!!!
gnoff Oct 5, 2022
843f7c4
restore preinit style flushing behavior and nits
gnoff Oct 17, 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
Prev Previous commit
restore preinit style flushing behavior and nits
  • Loading branch information
@gnoff
gnoff committed Oct 17, 2022
commit 843f7c4eeab55c6a8e24304a01bc914f3b2e0c5f
33 changes: 12 additions & 21 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,7 +34,6 @@ type ResourceType = 'style' | 'font' | 'script';

type PreloadProps = {
rel: 'preload',
as: ResourceType,
href: string,
[string]: mixed,
};
Expand All @@ -49,7 +48,7 @@ type PreloadResource = {
type StyleProps = {
rel: 'stylesheet',
href: string,
'data-rprec': string,
'data-precedence': string,
[string]: mixed,
};
export type StyleResource = {
Expand Down Expand Up @@ -333,7 +332,7 @@ function stylePropsFromPreinitOptions(
return {
rel: 'stylesheet',
href,
'data-rprec': precedence,
'data-precedence': precedence,
crossOrigin: options.crossOrigin,
};
}
Expand Down Expand Up @@ -363,7 +362,6 @@ type StyleQualifyingProps = {
type PreloadQualifyingProps = {
rel: 'preload',
href: string,
as: ResourceType,
[string]: mixed,
};
type ScriptQualifyingProps = {
Expand Down Expand Up @@ -443,8 +441,8 @@ export function getResource(
if (__DEV__) {
validateLinkPropsForPreloadResource(pendingProps);
}
const {href, as} = pendingProps;
if (typeof href === 'string' && isResourceAsType(as)) {
const {href} = pendingProps;
if (typeof href === 'string') {
// We've asserted all the specific types for PreloadQualifyingProps
const preloadRawProps: PreloadQualifyingProps = (pendingProps: any);
let resource = preloadResources.get(href);
Expand Down Expand Up @@ -539,7 +537,7 @@ function preloadPropsFromRawProps(

function stylePropsFromRawProps(rawProps: StyleQualifyingProps): StyleProps {
const props: StyleProps = Object.assign({}, rawProps);
props['data-rprec'] = rawProps.precedence;
props['data-precedence'] = rawProps.precedence;
props.precedence = null;

return props;
Expand Down Expand Up @@ -804,7 +802,7 @@ function acquireStyleResource(resource: StyleResource): Instance {
props.href,
);
const existingEl = root.querySelector(
`link[rel="stylesheet"][data-rprec][href="${limitedEscapedHref}"]`,
`link[rel="stylesheet"][data-precedence][href="${limitedEscapedHref}"]`,
);
if (existingEl) {
resource.instance = existingEl;
Expand Down Expand Up @@ -937,12 +935,14 @@ function insertStyleInstance(
precedence: string,
root: FloatRoot,
): void {
const nodes = root.querySelectorAll('link[rel="stylesheet"][data-rprec]');
const nodes = root.querySelectorAll(
'link[rel="stylesheet"][data-precedence]',
);
const last = nodes.length ? nodes[nodes.length - 1] : null;
let prior = last;
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i];
const nodePrecedence = node.dataset.rprec;
const nodePrecedence = node.dataset.precedence;
if (nodePrecedence === precedence) {
prior = node;
} else if (prior !== last) {
Expand Down Expand Up @@ -1009,13 +1009,8 @@ export function isHostResourceType(type: string, props: Props): boolean {
);
}
case 'preload': {
const {href, as, onLoad, onError} = props;
return (
!onLoad &&
!onError &&
typeof href === 'string' &&
isResourceAsType(as)
);
const {href, onLoad, onError} = props;
return !onLoad && !onError && typeof href === 'string';
}
}
return false;
Expand All @@ -1030,10 +1025,6 @@ export function isHostResourceType(type: string, props: Props): boolean {
return false;
}

function isResourceAsType(as: mixed): boolean {
return as === 'style' || as === 'font' || as === 'script';
}

// When passing user input into querySelector(All) the embedded string must not alter
// the semantics of the query. This escape function is safe to use when we know the
// provided value is going to be wrapped in double quotes as part of an attribute selector
Expand Down
8 changes: 4 additions & 4 deletions 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 @@ -903,15 +903,15 @@ function getNextHydratable(node) {
const rel = linkEl.rel;
if (
rel === 'preload' ||
(rel === 'stylesheet' && linkEl.hasAttribute('data-rprec'))
(rel === 'stylesheet' && linkEl.hasAttribute('data-precedence'))
) {
continue;
}
break;
}
case 'STYLE': {
const styleEl: HTMLStyleElement = (element: any);
if (styleEl.hasAttribute('data-rprec')) {
if (styleEl.hasAttribute('data-precedence')) {
continue;
}
break;
Expand Down Expand Up @@ -942,15 +942,15 @@ function getNextHydratable(node) {
const rel = linkEl.rel;
if (
rel === 'preload' ||
(rel === 'stylesheet' && linkEl.hasAttribute('data-rprec'))
(rel === 'stylesheet' && linkEl.hasAttribute('data-precedence'))
) {
continue;
}
break;
}
case 'STYLE': {
const styleEl: HTMLStyleElement = (element: any);
if (styleEl.hasAttribute('data-rprec')) {
if (styleEl.hasAttribute('data-precedence')) {
continue;
}
break;
Expand Down
36 changes: 14 additions & 22 deletions packages/react-dom-bindings/src/server/ReactDOMFloatServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type PreloadResource = {
type StyleProps = {
rel: 'stylesheet',
href: string,
'data-rprec': string,
'data-precedence': string,
[string]: mixed,
};
type StyleResource = {
Expand All @@ -52,6 +52,7 @@ type StyleResource = {
flushed: boolean,
inShell: boolean, // flushedInShell
hint: PreloadResource,
set: Set<StyleResource>, // the precedence set this resource should be flushed in
};

type ScriptProps = {
Expand Down Expand Up @@ -236,19 +237,18 @@ function preinit(href: string, options: PreinitOptions) {
const as = options.as;
switch (as) {
case 'style': {
const precedence = options.precedence || 'default';

let resource = resources.stylesMap.get(href);
if (resource) {
if (__DEV__) {
const latestProps = stylePropsFromPreinitOptions(
href,
precedence,
resource.precedence,
options,
);
validateStyleResourceDifference(resource.props, latestProps);
}
} else {
const precedence = options.precedence || 'default';
const resourceProps = stylePropsFromPreinitOptions(
href,
precedence,
Expand All @@ -261,6 +261,7 @@ function preinit(href: string, options: PreinitOptions) {
resourceProps,
);
}
resource.set.add(resource);
resources.explicitStylePreloads.add(resource.hint);

return;
Expand Down Expand Up @@ -380,7 +381,7 @@ function stylePropsFromRawProps(
const props: StyleProps = Object.assign({}, rawProps);
props.href = href;
props.rel = 'stylesheet';
props['data-rprec'] = precedence;
props['data-precedence'] = precedence;
delete props.precedence;

return props;
Expand All @@ -394,7 +395,7 @@ function stylePropsFromPreinitOptions(
return {
rel: 'stylesheet',
href,
'data-rprec': precedence,
'data-precedence': precedence,
crossOrigin: options.crossOrigin,
};
}
Expand All @@ -416,8 +417,10 @@ function createStyleResource(

// If this is the first time we've seen this precedence we encode it's position in our set even though
// we don't add the resource to this set yet
if (!precedences.has(precedence)) {
precedences.set(precedence, new Set());
let precedenceSet = precedences.get(precedence);
if (!precedenceSet) {
precedenceSet = new Set();
precedences.set(precedence, precedenceSet);
}

let hint = preloadsMap.get(href);
Expand Down Expand Up @@ -457,6 +460,7 @@ function createStyleResource(
inShell: false,
props,
hint,
set: precedenceSet,
};
stylesMap.set(href, resource);

Expand Down Expand Up @@ -633,14 +637,7 @@ export function resourcesFromLink(props: Props): boolean {
if (resources.boundaryResources) {
resources.boundaryResources.add(resource);
} else {
// Precedences are constructed eagerly when encountered so this will always exist
// Note that it is important to read the precedence from the resource. It is possible
// that the props precedence is new and does not match the precedence on an earlier
// constructed version of this resource.
const set: Set<StyleResource> = (resources.precedences.get(
resource.precedence,
): any);
set.add(resource);
resource.set.add(resource);
}
return true;
}
Expand Down Expand Up @@ -765,11 +762,6 @@ export function hoistResourcesToRoot(
resources: Resources,
boundaryResources: BoundaryResources,
): void {
const {precedences} = resources;
boundaryResources.forEach(resource => {
// all precedences are set upon discovery. so we know we will have a set here
const set: Set<StyleResource> = (precedences.get(resource.precedence): any);
set.add(resource);
});
boundaryResources.forEach(resource => resource.set.add(resource));
boundaryResources.clear();
}
78 changes: 24 additions & 54 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 @@ -2259,7 +2259,7 @@ function escapeJSObjectForInstructionScripts(input: Object): string {
}

const precedencePlaceholderStart = stringToPrecomputedChunk(
'<style data-rprec="',
'<style data-precedence="',
);
const precedencePlaceholderEnd = stringToPrecomputedChunk('"></style>');

Expand All @@ -2268,6 +2268,13 @@ export function writeInitialResources(
resources: Resources,
responseState: ResponseState,
): boolean {
function flushLinkResource(resource) {
if (!resource.flushed) {
pushLinkImpl(target, resource.props, responseState);
resource.flushed = true;
}
}

const target = [];

const {
Expand Down Expand Up @@ -2307,13 +2314,7 @@ export function writeInitialResources(
}
});

usedStylePreloads.forEach(r => {
// style preloads could very likely have been flushed already so we check
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
usedStylePreloads.forEach(flushLinkResource);
usedStylePreloads.clear();

scripts.forEach(r => {
Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 15, 2022

Choose a reason for hiding this comment

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

The preamble notes didn't have anything about scripts in it.

I think we said that we don't want to emit scripts in the preamble, except the external fizz runtime. Because they can end up flooding the network ahead of the styles that are going to come soon.

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 15, 2022

Choose a reason for hiding this comment

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

They're not needed for display. The exception is the fizz runtime which will be needed to display boundaries.

Copy link
Collaborator Author

@gnoff gnoff Oct 17, 2022

Choose a reason for hiding this comment

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

Keep in mind there is not preamble flush yet because we haven't solved the issue of returning the stream before onReady. So we effectively don't have a preamble flush yet and the first flush is following shell priority

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 17, 2022

Choose a reason for hiding this comment

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

Seems like we should prep the code for it though. Otherwise, what's the difference between Immediate and Initial?

Copy link
Collaborator Author

@gnoff gnoff Oct 17, 2022

Choose a reason for hiding this comment

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

we don't flush styles in immediate

Expand All @@ -2325,29 +2326,13 @@ export function writeInitialResources(
});
scripts.clear();

usedScriptPreloads.forEach(r => {
// may have been flushed so we check
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
usedScriptPreloads.forEach(flushLinkResource);
usedScriptPreloads.clear();

explicitStylePreloads.forEach(r => {
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
explicitStylePreloads.forEach(flushLinkResource);
explicitStylePreloads.clear();

explicitScriptPreloads.forEach(r => {
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
explicitScriptPreloads.forEach(flushLinkResource);
explicitScriptPreloads.clear();

let i;
Expand All @@ -2366,6 +2351,13 @@ export function writeImmediateResources(
resources: Resources,
responseState: ResponseState,
): boolean {
function flushLinkResource(resource) {
if (!resource.flushed) {
pushLinkImpl(target, resource.props, responseState);
resource.flushed = true;
}
}

const target = [];

const {
Expand All @@ -2384,13 +2376,7 @@ export function writeImmediateResources(
});
fontPreloads.clear();

usedStylePreloads.forEach(r => {
// style preloads could very likely have been flushed already so we check
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
usedStylePreloads.forEach(flushLinkResource);
usedStylePreloads.clear();

scripts.forEach(r => {
Expand All @@ -2402,29 +2388,13 @@ export function writeImmediateResources(
});
scripts.clear();

usedScriptPreloads.forEach(r => {
// may have been flushed so we check
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
usedScriptPreloads.forEach(flushLinkResource);
usedScriptPreloads.clear();

explicitStylePreloads.forEach(r => {
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
explicitStylePreloads.forEach(flushLinkResource);
explicitStylePreloads.clear();

explicitScriptPreloads.forEach(r => {
if (!r.flushed) {
pushLinkImpl(target, r.props, responseState);
r.flushed = true;
}
});
explicitScriptPreloads.forEach(flushLinkResource);
explicitScriptPreloads.clear();

let i;
Expand Down Expand Up @@ -2548,7 +2518,7 @@ function writeStyleResourceDependency(
case 'href':
case 'rel':
case 'precedence':
case 'data-rprec': {
case 'data-precedence': {
break;
}
case 'children':
Expand Down
Loading