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

Jump to bottom

Add option to load Fizz runtime from external file #25499

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Add option to load Fizz runtime from external file #25499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6a5304
Add feature flag for external Fizz runtime
acdlite Oct 17, 2022
68a25c1
Add option to load Fizz runtime from external file
acdlite 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
25 changes: 25 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 @@ -23,6 +23,7 @@ import {
enableFilterEmptyStringAttributesDOM,
enableCustomElementPropertySupport,
enableFloat,
enableFizzExternalRuntime,
} from 'shared/ReactFeatureFlags';

import type {
Expand Down Expand Up @@ -156,6 +157,7 @@ export function createResponseState(
bootstrapScriptContent: string | void,
bootstrapScripts: $ReadOnlyArray<string | BootstrapScriptDescriptor> | void,
bootstrapModules: $ReadOnlyArray<string | BootstrapScriptDescriptor> | void,
externalRuntimeConfig: string | BootstrapScriptDescriptor | void,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense to support the same configuration options as bootstrapScripts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Style / naming - would it make sense to name something this like staticRuntimeConfig : StaticRuntimeConfig and have an explicit src? Not familiar with this codebase's style, so please ignore if this contradicts

type StaticRuntimeConfig = {
  src: string | BootstrapScriptDescriptor,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure, I'll defer to @gnoff and @sebmarkbage on the API shape. My best guess was to copy the convention for the other options, since it makes sense to me that they would be consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the BootstrapScriptDescriptor isn't a src type so I wouldn't factor it that way.

I would probably just rename it to externalRuntimScript and leave the type as is. if it is a string it is implicitly src. if you pass an object it needs to have a src.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, we prefer few objects for perf.

): ResponseState {
const idPrefix = identifierPrefix === undefined ? '' : identifierPrefix;
const inlineScriptWithNonce =
Expand All @@ -172,6 +174,29 @@ export function createResponseState(
endInlineScript,
);
}
if (enableFizzExternalRuntime) {
if (externalRuntimeConfig !== undefined) {
const src =
typeof externalRuntimeConfig === 'string'
? externalRuntimeConfig
: externalRuntimeConfig.src;
const integrity =
typeof externalRuntimeConfig === 'string'
? undefined
: externalRuntimeConfig.integrity;
bootstrapChunks.push(
startScriptSrc,
stringToChunk(escapeTextForBrowser(src)),
);
if (integrity) {
bootstrapChunks.push(
scriptIntegirty,
stringToChunk(escapeTextForBrowser(integrity)),
);
}
bootstrapChunks.push(endAsyncScript);
Comment on lines +187 to +197
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause the runtime to emit after all the content. The right priority is probably much higher. I just merged support for scripts but we probably want to special case this one as the highest priority script to flush. I'm mostly just flagging it here not so that it gets changed in this PR but that I follow up in my own PR afterwards

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I can reorder it if you want. Or leave it for you to change.

}
}
if (bootstrapScripts !== undefined) {
for (let i = 0; i < bootstrapScripts.length; i++) {
const scriptConfig = bootstrapScripts[i];
Expand Down
30 changes: 30 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3546,6 +3546,36 @@ describe('ReactDOMFizzServer', () => {
});
});

// @gate enableFizzExternalRuntime
it('supports option to load runtime as an external script', async () => {
await actIntoEmptyDocument(() => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
<html>
<head />
<body>
<div>hello world</div>
</body>
</html>,
{
unstable_externalRuntimeSrc: 'src-of-external-runtime',
},
);
pipe(writable);
});

expect(getVisibleChildren(document)).toEqual(
<html>
<head />
<body>
<div>hello world</div>
</body>
</html>,
);
expect(
Array.from(document.getElementsByTagName('script')).map(n => n.outerHTML),
).toEqual(['<script src="src-of-external-runtime" async=""></script>']);
});

it('#24384: Suspending should halt hydration warnings and not emit any if hydration completes successfully after unsuspending', async () => {
const makeApp = () => {
let resolve, resolved;
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactDOMFizzServerBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Options = {
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
};

// TODO: Move to sub-classing ReadableStream.
Expand Down Expand Up @@ -86,6 +87,7 @@ function renderToReadableStream(
options ? options.bootstrapScriptContent : undefined,
options ? options.bootstrapScripts : undefined,
options ? options.bootstrapModules : undefined,
options ? options.unstable_externalRuntimeSrc : undefined,
),
createRootFormatContext(options ? options.namespaceURI : undefined),
options ? options.progressiveChunkSize : undefined,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactDOMFizzServerNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ type Options = {
onShellError?: (error: mixed) => void,
onAllReady?: () => void,
onError?: (error: mixed) => ?string,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
};

type PipeableStream = {
Expand All @@ -65,6 +66,7 @@ function createRequestImpl(children: ReactNodeList, options: void | Options) {
options ? options.bootstrapScriptContent : undefined,
options ? options.bootstrapScripts : undefined,
options ? options.bootstrapModules : undefined,
options ? options.unstable_externalRuntimeSrc : undefined,
),
createRootFormatContext(options ? options.namespaceURI : undefined),
options ? options.progressiveChunkSize : undefined,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactDOMFizzStaticBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type Options = {
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
};

type StaticResult = {
Expand Down Expand Up @@ -71,6 +72,7 @@ function prerender(
options ? options.bootstrapScriptContent : undefined,
options ? options.bootstrapScripts : undefined,
options ? options.bootstrapModules : undefined,
options ? options.unstable_externalRuntimeSrc : undefined,
),
createRootFormatContext(options ? options.namespaceURI : undefined),
options ? options.progressiveChunkSize : undefined,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/server/ReactDOMFizzStaticNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Options = {
progressiveChunkSize?: number,
signal?: AbortSignal,
onError?: (error: mixed) => ?string,
unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor,
};

type StaticResult = {
Expand Down Expand Up @@ -86,6 +87,7 @@ function prerenderToNodeStreams(
options ? options.bootstrapScriptContent : undefined,
options ? options.bootstrapScripts : undefined,
options ? options.bootstrapModules : undefined,
options ? options.unstable_externalRuntimeSrc : undefined,
),
createRootFormatContext(options ? options.namespaceURI : undefined),
options ? options.progressiveChunkSize : undefined,
Expand Down
3 changes: 3 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ export const enableUseMemoCacheHook = __EXPERIMENTAL__;

export const enableUseEventHook = __EXPERIMENTAL__;

// Test in www before enabling in open source.
export const enableFizzExternalRuntime = false;

// -----------------------------------------------------------------------------
// Chopping Block
//
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export const enableFloat = false;
export const enableHostSingletons = false;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be true instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is dead code tbh. I don't think we use it anywhere in www. It's a vestige from when we momentarily thought we would provide a special testing build of React. Need to delete, just haven't yet.


// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export const enableUseMutableSource = true;
export const enableCustomElementPropertySupport = __EXPERIMENTAL__;

export const useModernStrictMode = false;
export const enableFizzExternalRuntime = true;

// Flow magic to verify the exports of this file match the original version.
((((null: any): ExportsType): FeatureFlagsType): ExportsType);