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

RA-1976: Referring to jQuery as $j does not work in the new UI #100

Closed
wants to merge 1 commit into from

Conversation

mogoodrich
Copy link
Member

No description provided.

@mogoodrich
Copy link
Member Author

This is a hack due to the case that we have referencing to jQuery using both "jq" ad "$j" littered throughout out code, but I don't think there would be any negative ramifications of doing this?

Copy link
Contributor

@brandones brandones left a comment

Choose a reason for hiding this comment

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

Yeah, there should be no problems. Thanks @mogoodrich .

Copy link
Member

@mseaton mseaton left a comment

Choose a reason for hiding this comment

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

I honestly don't know the implications of this - I think we need to be very careful about this. Why exactly is this coming up now? I think it would be better to just fix whatever is using $j to use jq where appropriate.

@mogoodrich
Copy link
Member Author

@mseaton I'd not sure why this is just coming up now (and only for the Peru form)... I will dig a little deeper but changing all the $j to jq seemed a bigger risk... in particular, I suspect that will break things on users of HFE and the 1.x UI. I couldn't think of why my change would cause a problem unless someone was setting "$j" to something other than jquery elsewhere, or was setting it to a different version of jquery?

@brandones
Copy link
Contributor

I mean, the fact that any piece of code is attempting to use jQuery via an abbreviation that it doesn't guarantee is defined is nuts—whether that abbreviation is jq or $j or even $. In the absence of the kind of technology that can load explicitly declared dependencies, mandating that everything assume only jQuery is defined is probably the least bad option. This just means adding lines like const jq = jQuery or const $j = jQuery at the top of the scripts that use them.

But really, there's no risk associated with this, unless we are doing the truly insane: monkey-patching functions into jQuery. I doubt that's happening, so there really shouldn't be any implications to worry about.

@brandones
Copy link
Contributor

I guess the other thing that could go wrong is if there are multiple versions of jQuery. Like jQuery, $j, or jq refer to different versions of the library. You can check that by running jQuery.fn.jquery in the browser console, swapping out that first jQuery for the symbol that you want to check the referent library's version for.

@mogoodrich mogoodrich closed this Nov 22, 2021
@mogoodrich
Copy link
Member Author

Updated comment coming...

@mogoodrich
Copy link
Member Author

So I researched this a little more and there does seem a somewhat better place to add this, though I think it's crazy we got this far along without noticing this before... see my comment on the ticket (which I moved back into the HFE project):

https://issues.openmrs.org/browse/HTML-800

Testing now, will likely just merge if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants