Skip to content

Conversation

@cammonro
Copy link
Contributor

Summary

This PR aims to clean up the monolithic JS implementation with its heavy use of broadly scoped variables and closures and scattered implementation. We historically have had hooks to help customize the way the JavaScript works but it's very limited. Now it will be possible to write a mixin on any function to avoid overwriting the entire JS file.

  • All logic has been split granularly into named, mixable functions
  • Functions have been named in order to distinguish between InstantSearch widget objects, special facet config objects and builders
  • XSS vulnerabilities were identified and corrected
  • Behavior of conjunctive vs disjunctive facets has been clarified
  • InstantSearch lib has been upgraded to v4.78
  • Source maps have been added to InstantSearch to assist with debugging
  • jQuery dependency has been significantly reduced (still not there - more work to do here but the goal is to remove altogether)
  • Non relevant legacy code has been removed
  • A discovered JS bug with empty hits has been resolved (this was impacting the execution chain and creating some weird behaviors with other widgets)
  • Deprecation notices added - including UA sunsetted July 1, 2023
  • Facet building operations have been refactored for clarity of intent
  • A new front end hook called beforeFacetInitialization has been added which allows a user to extend the functionality by adding, removing or modifying "builder" functions which are used to define a facet config that will drive the rendering of facets

Result

Unit tests need to be added for this large revamp but manual testing has been performed including (but not limited to):

  • General functionality
    image
  • XSS (and suggestions custom widget)
    image
  • nodeValue undefined bug fix
    image
    image
  • Conjunctive facets (facet count change)
    image
    image
  • Disjunctive facets (no facet count change)
    image
    image
  • URL redirect on add to cart
    image

@cammonro cammonro added the WIP Work In Progress, do not merge / close yet label Mar 13, 2025
@cammonro cammonro requested a review from damcou March 13, 2025 14:38
@cammonro cammonro removed the WIP Work In Progress, do not merge / close yet label Mar 13, 2025
@cammonro cammonro marked this pull request as ready for review March 13, 2025 14:38
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

Wonderful work ! 🥳 💪

I really like how the file looks now, this is a lot easier to understand, even for a non-js developer ;) .
Added a few comments but nothing blocking for you to go forward.

Out of curiosity, have you considered splitting the file into multiple ones since this is still a 1000+ lines one ? This isn't mandatory for sure, I guess this has its own pros.

@cammonro
Copy link
Contributor Author

Thanks for the feedback! I'll pick up these observations on the next PR. 👍

Out of curiosity, have you considered splitting the file into multiple ones since this is still a 1000+ lines one ? This isn't mandatory for sure, I guess this has its own pros.

I did consider this! What's holding me back is that I would like to make these components available as UMD modules at some point for applications outside of Luma / RequireJS (like Hyvä etc.). Having too many dependencies can make that cumbersome unless we implement a build process for that (and we just ditched the build process necessary for algoliaBundle in the last release).

For now, I'm keeping the monolithic approach but just adding a bit of structure and scoping because before it was extremely brittle. Happy to revisit this in the future - but hopefully this is a step in the right direction.

@cammonro cammonro merged commit 40c0c3b into release/3.16.0-dev Mar 14, 2025
3 checks passed
@cammonro cammonro deleted the feat/MAGE-974-extension-points branch March 14, 2025 13:25
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

Successfully merging this pull request may close these issues.

3 participants