SLING-11087: Fix for multiple components being fetched via JSI #23

jar-miracle · 2022-01-25T10:03:34Z

Bugfix: https://issues.apache.org/jira/browse/SLING-11087

Fixes scope related issues in JSI causing only one component to be shown in cases where multiple components should be fetched asynchronously.

rombert

Thanks for the contributions! I don't think we really have someone familiar with the module ready to review, so I'll give this a shot.

My only question is whether using let instead of var is safe from the point of view of browser support. If that is not the case, have you considered using an IIFE?

jar-miracle · 2022-01-26T07:32:14Z

Seems let is supported by most major browsers since at least 2016: https://caniuse.com/let

Using var pollutes the global variable space of the browser. This causes the same component to be inserted multiple times rather than different components. I don't know too much about JavaScript, but I don't see how using var along with IIFE would solve this issue.

Another solution could be to append the xhr variable with a number similar to how the div is appended. We went with the let solution at the company where I needed this.

jsedding · 2022-01-26T09:37:38Z

Personally, I would be slightly more comfortable with the IIFE approach. Not because it is better, but because it conveys the intent about encapsulation of the scope more clearly than. While I am sure you are correct and let stays within the scope of the if statement, it does require background knowledge about a subtlety of the javascript programming language. Given Sling is predominantly Java, I don't think it is safe to assume this knowledge as a given.

jar-miracle · 2022-01-26T09:42:23Z

The primary issue is that it should stay within the <script>-scope. If multiple components are to be replaced you have multiple <script> sections with the same variables. Using var here causes issues. We can use IIFE, no problem, but that's a further change to functionality than just fixing the bug.

I will update the PR however when I get time to test it properly as well.

This reverts commit 531ffc1.

jar-miracle · 2022-01-26T11:24:21Z

Upon testing I agree your proposal is much cleaner. As I fall into the previously mentioned Java group of Sling developers I am not too sure about browser support for the anonymous function used in the IIFE pattern. Reverted previous commit and introduced an IIFE-based solution.