search

ynorth-projects / openy_repeat

Decouple openy_repeat.
1 stars 17 forks source link

Avoid modifying Array.prototype #66

Open andriokha opened 2 months ago

andriokha commented 2 months ago

Hi, thanks for the layout builder support, we have a happy YMCA (:

However we noticed when we added it to a branch page that the map stopped working. I seem to have narrowed it down to the geolocation module's JS not playing nicely with library-davekoelle/alphanum updating Array.prototype. I've added an MR to this geolocation issue that solves the problem I'm facing (see this explanation), but I thought it might also be good to program this module defensively and not pollute the global Array.prototype, at least while it's still common for folk to use jQuery.extend(). Could we maybe namespace it to window.OpenY.something?

Thanks!

podarok commented 2 months ago

window.ws.something sounds ok @andriokha How much time it would take to have it?

andriokha commented 2 months ago

Thanks @podarok. It seems to me the next steps would be:

  1. Understand everywhere that uses the library. On my site it seems only to be used by this module, but the dependency actually comes from ycloudyusa/yusaopeny, so I don't know if it isn't used by other optional packages I don't have installed. We can use github search, but I know there are multiple organizations to search under, and I don't know them all.
  2. Decide how to include the functionality from library-davekoelle/alphanum. Are we going to just fold it into the existing codebase (in which case into which project)? It's MIT-licensed, so I think that would be fine from a license perspective. Alternatively we could create a standalone dependency for it (in which case do we still want ycloudyusa/yusaopeny to require it, or just the projects that use it?). Edit: A simpler and more brittle approach would be to do it dynamically in JS, ie 
    const alphanumSort = Array.prototype.alphanumSort;
delete Array.prototype.alphanumSort;
window.ws.alphanumSort = array => alphanumSort.call(array);

    But that would need strict ordering to ensure it executed after the sort was added but before anything tried to use it. We could also take the opportunity to create a sort function that doesn't modify the array in place, but returns a copy. That's a stylistic thing, dunno what you'd prefer.

  3. Update the dependencies and calling code based on previous decisions.
  4. Mention in the release notes that anyone relying on the presence of Array.prototype.alphanumSort() will need to either require the library themselves or use the WS location.