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.

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

refactor(platform-server): minor cleanup in insertEventRecordScript logic #55790

Closed

Conversation

alan-agius4
Copy link
Contributor

  • Uses Set instead of a number of checks
  • Remove unneeded Array.from

@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels May 14, 2024
@alan-agius4 alan-agius4 added the area: server Issues related to server-side rendering label May 14, 2024
@ngbot ngbot bot added this to the Backlog milestone May 14, 2024
@alan-agius4 alan-agius4 force-pushed the insertEventRecordScript-refactor branch 4 times, most recently from 1ad5994 to 4e98a22 Compare May 14, 2024 14:36
… logic

- Uses Set instead of a number of checks
- Remove unneeded `Array.from`
@alan-agius4 alan-agius4 force-pushed the insertEventRecordScript-refactor branch from 4e98a22 to 232dc77 Compare May 14, 2024 15:29
'pointerenter',
'pointerleave',
]);
const ALLOWED_CAPTAURE_EVENT_TYPES = new Set<string>(['focus', 'blur', 'error', 'load', 'toggle']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const ALLOWED_CAPTAURE_EVENT_TYPES = new Set<string>(['focus', 'blur', 'error', 'load', 'toggle']);
const ALLOWED_CAPTURE_EVENT_TYPES = new Set<string>(['focus', 'blur', 'error', 'load', 'toggle']);

if (DISALLOWED_EVENT_TYPES.has(eventType)) {
continue;
}
if (ALLOWED_CAPTAURE_EVENT_TYPES.has(eventType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (ALLOWED_CAPTAURE_EVENT_TYPES.has(eventType)) {
if (ALLOWED_CAPTURE_EVENT_TYPES.has(eventType)) {

@@ -133,49 +133,56 @@ function appendServerContextInfo(applicationRef: ApplicationRef) {
});
}

const DISALLOWED_EVENT_TYPES = new Set<string>([
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like those consts would intersect with a similar ones from PR #55799.

Comment on lines +176 to +177
if (captureEventTypes.length) {
jsActionBootstrapParams.push(JSON.stringify(captureEventTypes));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
if (captureEventTypes.length) {
jsActionBootstrapParams.push(JSON.stringify(captureEventTypes));
if (captureEventTypes.length > 0) {
jsActionBootstrapParams.push(JSON.stringify(captureEventTypes));


const replayScript = createScript(doc, replayScriptContents, nonce);
// This is defined in packages/core/primitives/event-dispatch/contract_binary.ts
const jsActionBootstrapParams = [
Copy link
Contributor

Choose a reason for hiding this comment

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

This change would also conflict with the changes from #55799. Since #55799 touches the same code, we can either wait for that PR to land or provide more feedback in #55799, so this change gets incorporated there.

@AndrewKushnir AndrewKushnir added target: patch This PR is targeted for the next patch release and removed target: rc This PR is targeted for the next release-candidate labels May 22, 2024
@alan-agius4 alan-agius4 deleted the insertEventRecordScript-refactor branch May 28, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: server Issues related to server-side rendering target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants