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

fix: workaround for the cascading parameters bug #357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hossted-prs
Copy link

@hossted-prs hossted-prs commented Oct 15, 2024

Changes (taken from commit message)

  • addresses the issue with the rendering order of the dependent parameters
  • the workaround performs the following:
    • topologically sorts the parameters, ensuring they are updated in the correct order
    • tracks the updates with the clever trick which tracks the use log messages (parses the log messages to determine the status of the update(s))
    • listens for changes to the parameters and ensures proper propagation of values to their dependencies
    • (optionally) handles the visibility during updates to improve the user experience

The issue

JENKINS-73935 (links to multiple similar reported issues)

Commentary

This is a workaround. The project maintainers will more than likely reject this actual PR itself (since it appends logic to the original logic, creating a minor mess), but the goal of this PR submission is to start a discussion and bug resolution process regarding the described ticket listed above.
We want to inspire the maintainers with our approach on solving the issue.

NOTE: Keep in mind that in our original fix, we actually append the code to the transpiled JavaScript file (adjuncts/<xxxxxxxx>/org/biouno/unochoice/stapler/unochoice/UnoChoice.js), not to this file. But still, the code was appended to this source file just for symbolic reasons. Therefore, the tests are expected to fail.

In the provided code, you can see how our team approached solving the issue with a somewhat unconventional workaround, but the end goal is achieved - we resolved the issue related to the cascading parameters ordering bug.

Feel free to refer to the ticket.
Additionally, feel free to check out our dedicated repository containing the fix as well as a build script used for producing a patched variant of the plugin bundle:
hossted-prs/jenkins-ac-plugin-cascading-workaround

* addresses the issue with the rendering order of the dependent parameters
* the workaround performs the following:
  * topologically sorts the parameters, ensuring they are updated in the correct
    order
  * tracks the updates with the clever trick which tracks the use log messages
    (parses the log messages to determine the status of the update(s))
  * listens for changes to the parameters and ensures proper propagation of
    values to their dependencies
  * (optionally) handles the visibility during updates to improve the user
    experience
@hossted-prs hossted-prs requested a review from a team as a code owner October 15, 2024 11:57
@hossted-prs
Copy link
Author

hossted-prs commented Oct 15, 2024

As stated above, please note that the failing tests are to be expected. The goal of this PR is not to get merged. Instead, we want to demonstrate how we approached in solving the issue.

We would like to comment on our approach for this issue:

  • we noticed that the issue occurs because whenever the parameter object triggers and update via the update() method, the method function prematurely returns instead of waiting for each parameter to finish updating
  • we approached this issue by patching the console log function (restoring it to original afterwards) and parsing the console log messages, since it was the only reliable & stable way to identify when did parameters finish updating
  • ideally, of course, we want to avoid such a "hacky" way of identifying when the parameter(s) finished updating
  • the root cause of this issue is update() method returning prematurely and executing the updating logic asynchronously (on the event loop)
  • in the older version(s) of the related plugin code (pre-2.7), no asynchronous code seems to have been used for the updating logic - this gives us strong suspicion that it is indeed an asynchronous logic problem (devs commonly misuse Promise objects, or accidentally issue callbacks which result in instant function return instead of waiting for the actual call logic to finish)

@kinow
Copy link
Member

kinow commented Oct 15, 2024

Hi

Thanks for providing a workaround and starting the discussion, even if it doesn't necessarily fix the issue, it may point us at the right direction.

After the latest release (a few days ago?) I promised other users of tap plugin to fix some issues there and maybe cut a tap release too.

Will come back to this one after that is done. For now, what I might say is this to whoever worked on the issue:

  • I will have a look at the sorting hierarchical ... I planned to actually reuse something that does that like Vue or its reactivity system, but if your approach works then even better/simpler!
  • the approach with parsing console log (if I understood correctly from the description) definitely needs to go... nut maybe we can use a global variable?
  • the point about the root cause... thanks a lot! As I work on multiple open source projects on spare time now (company where I work use GitLab and HPC tools) when I come back to work on this code sometimes I have little recollection of what was missing and where... will read your changes with care in one week or so (busy with something else, before tap plugin works start) to read the code and maybe leave some other comments here. Thanks!!
  • and yup, the change to asynchronous code broke reactivity for sure, but I only realized that after a user provided an example code to reproduce it. It is now undone.

Cheers

@kinow
Copy link
Member

kinow commented Oct 15, 2024

(And great job!!)

@hossted-prs
Copy link
Author

hossted-prs commented Oct 15, 2024

You are welcome!

Looking forward to the resolution of this issue and we hope that these insights were at least of some value to you.
Also, we appreciate your efforts in maintaining the plugin.
Thanks!

Best regards,
Hossted

@liorkesos
Copy link

liorkesos commented Oct 15, 2024

Hi @kinow , We sponsored the work on this on behalf of our client , @matijaaa did an awesome job developing the workaround and submitting the code for the PR .
We appreciate the work you've put in this plugin (for almost a decade!) and it has become a critical part of our client's workflow.
@hossted-prs and @matijaaa will be happy to continue any testing or interaction needed to get this committed upstream.

@QcFe
Copy link

QcFe commented Oct 16, 2024

I'm not extremely sure this is related but I had a similar issue with reactive parameters not being updated when I type stuff in the filter. It actually gets fixed for the current page if I input an easy peasy window.$ = jQuery inside the browser console... Maybe the $ alias for jQuery has been removed at some point?

@kinow
Copy link
Member

kinow commented Oct 16, 2024

I'm not extremely sure this is related but I had a similar issue with reactive parameters not being updated when I type stuff in the filter. It actually gets fixed for the current page if I input an easy peasy window.$ = jQuery inside the browser console... Maybe the $ alias for jQuery has been removed at some point?

Long story short, Jenkins dropped jquery but we add it via a plugin. The issue is that the work to make the plugin asynchronous also had to deal with the jquery removal.

I disabled asynchronous calls and tried to revert the calls to $ to use
.. jquery or jquery3 is now the global available in the plugin context (not sure if afterwards window gets another or the same variable).

So your issue might be something I forgot to fix, @QcFe , however, I will have a look at this too. You can never be too sure, and your comment might be a useful clue to fix the issue. Thanks for the comment!! 🙌

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Hi! Just passing by to say I haven't forgotten about this PR. There were multiple pending PRs from dependabot and contributors, which I have just finished reviewing and merging everything. 2.8.5 is being released right now, and then we can rebase and start over here to review without the other changes. Sorry the delay, wait a bit more and I hope to come back with a more useful update 👍

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.

5 participants