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

Bug: mergeH5PIntegration duplicates core scripts and styles when player is opened #3471

Open
TylerZeroMaster opened this issue Jan 2, 2024 · 0 comments

Comments

@TylerZeroMaster
Copy link

Detailed explanation

Hello, I ran into a minor problem with the H5PPlayer. When the player opens, mergeH5PIntegration "[...] merge[s] the H5P integration we received from the server with the one that already exists in the window globally to allow for several H5P content objects on a single page." When this merge happens, the contents of core.styles and core.scripts are duplicated. This duplication happens each time the player renders.

This can be a problem because:

  1. The arrays grow continuously (memory leak)
  2. H5P.getHeadTags pulls directly from H5PIntegration.core and does not check for duplication
  3. Scripts execute multiple times (can be problematic for editor and player customizations)

Steps to reproduce

  1. Goto wherever your ContentList is rendered
  2. Open and close the player a few times without reloading the page
  3. With the player open, inspect the head tag within the player component to find duplicated script tags
Screenshot 2024-01-02 at 9 47 28 AM

Possible fix

deepmerge, the library used to merge objects, concatenates arrays by default; however, it supports customization for array merging. I was able to get everything working as expected by implementing the following:

  1. Define that each item of an array should be merged (mostly a copy/paste from deepmerge documentation)
const combineMerge = <T extends Partial<unknown>>(
  target: T[],
  source: T[],
  options: {
    isMergeableObject: (item: T) => boolean;
    cloneUnlessOtherwiseSpecified: (item: T, options: any) => T;
  },
) => {
  const destination = target.slice();

  source.forEach((item, index) => {
    if (typeof destination[index] === 'undefined') {
      destination[index] = options.cloneUnlessOtherwiseSpecified(item, options);
    } else if (options.isMergeableObject(item)) {
      destination[index] = merge(target[index], item, options);
    } else if (target.indexOf(item) === -1) {
      destination.push(item);
    }
  });
  return destination;
};
  1. Use the new logic for merging arrays
export function mergeH5PIntegration(
// ...
  window.H5PIntegration = merge(window.H5PIntegration, newIntegrationDup, {
    arrayMerge: combineMerge,
  });
// ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant