Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-35883

Remove fetch and fetcher from JDL

    Details

    • Epic Link:
    • Sprint:
      1.0-japan-m9, 1.0-m10
    • Similar Issues:

      Description

      The HOC functionality for REST that lives in fetch.jsx and fetcher.jsx isn't bad code by any means, but it's a hack that should be retired now we've got Redux store functionality in the main app.

      If we can't replace it altogether yet, it should be at least moved back into blueocean-dashboard, as it has BO-specific details, dealing with the Config class. But ideally we can locate the few dependencies on it and upgrade them to work via Redux, and just dump it. Deleted code is the best code.

        Attachments

          Activity

          Hide
          sophistifunk Josh McDonald added a comment -

          We're also shadowing a global in modern browsers, which is never a good thing: https://github.com/jenkinsci/jenkins-design-language/blob/master/src/js/components/fetch.jsx#L12

          Show
          sophistifunk Josh McDonald added a comment - We're also shadowing a global in modern browsers, which is never a good thing: https://github.com/jenkinsci/jenkins-design-language/blob/master/src/js/components/fetch.jsx#L12
          Hide
          cliffmeyers Cliff Meyers added a comment -

          [~jmcdonald] it looks like LogConsole has a dependency on the fetcher right now. We don't have any other "smart" components in the JDL right now, so I agree it'd be best if we simplify this one. We could certainly rework this so it invokes an externally-supplied method and is supplied its data via props, as per usual. That would eliminate the fetch/fetcher dep. That sound right to you?

          Show
          cliffmeyers Cliff Meyers added a comment - [~jmcdonald] it looks like LogConsole has a dependency on the fetcher right now. We don't have any other "smart" components in the JDL right now, so I agree it'd be best if we simplify this one. We could certainly rework this so it invokes an externally-supplied method and is supplied its data via props, as per usual. That would eliminate the fetch/fetcher dep. That sound right to you?
          Hide
          sophistifunk Josh McDonald added a comment -

          Definitely. We want helpers and renderers in JDL, rather than anything that dictates stuff like server communication methods. I'd just move LogConsole into blueocean-dashboard wholesale, as it too is kind of a hack that needs to go away rather than be promoted to "reusable goodies". We thought it would be less of a performance issue than it turned out to be.

          Show
          sophistifunk Josh McDonald added a comment - Definitely. We want helpers and renderers in JDL, rather than anything that dictates stuff like server communication methods. I'd just move LogConsole into blueocean-dashboard wholesale, as it too is kind of a hack that needs to go away rather than be promoted to "reusable goodies". We thought it would be less of a performance issue than it turned out to be.
          Hide
          michaelneale Michael Neale added a comment -

          The log component as it is will change a lot, so perhaps this will just go away naturally

          Show
          michaelneale Michael Neale added a comment - The log component as it is will change a lot, so perhaps this will just go away naturally
          Hide
          cliffmeyers Cliff Meyers added a comment -

          [~mneale] [~jmcdonald] I thought that Thorsten Scherler was doing some rework to this component but I know he's out for at least another week too. If we wanna get the JDL cleaned up now, I could do the following:

          1. Migrate LogConsole with fetcher/fetch into blueocean-plugin as-is,
          2. Rework LogConsole to use isomorphic-fetch and Redux plumbing.

          I only hesitate slightly on part 2 'cause if Thor has already started majoring refactoring then it might turn out to be a dup of effort. If goes together quickly I'll just do it, but if I find myself burning more time than I'd like maybe I'll hold off and just submit a PR for part #1?

          Show
          cliffmeyers Cliff Meyers added a comment - [~mneale] [~jmcdonald] I thought that Thorsten Scherler was doing some rework to this component but I know he's out for at least another week too. If we wanna get the JDL cleaned up now, I could do the following: Migrate LogConsole with fetcher/fetch into blueocean-plugin as-is, Rework LogConsole to use isomorphic-fetch and Redux plumbing. I only hesitate slightly on part 2 'cause if Thor has already started majoring refactoring then it might turn out to be a dup of effort. If goes together quickly I'll just do it, but if I find myself burning more time than I'd like maybe I'll hold off and just submit a PR for part #1?
          Hide
          cliffmeyers Cliff Meyers added a comment -

          Yeah looks like Thor is already doing something similar here: https://github.com/jenkinsci/blueocean-plugin/pull/212/files

          Show
          cliffmeyers Cliff Meyers added a comment - Yeah looks like Thor is already doing something similar here: https://github.com/jenkinsci/blueocean-plugin/pull/212/files
          Hide
          sophistifunk Josh McDonald added a comment -

          I feel a disturbance in the force, as if millions of merge conflicts suddenly cried out... :-/

          Show
          sophistifunk Josh McDonald added a comment - I feel a disturbance in the force, as if millions of merge conflicts suddenly cried out... :-/
          Hide
          michaelneale Michael Neale added a comment -

          [~cmeyers] actually [~jmcdonald] was going to shoot you an email, in doing this, between you and josh may as well tidy up Thorstens branch and get it into master (check for email)

          Show
          michaelneale Michael Neale added a comment - [~cmeyers] actually [~jmcdonald] was going to shoot you an email, in doing this, between you and josh may as well tidy up Thorstens branch and get it into master (check for email)

            People

            • Assignee:
              tscherler Thorsten Scherler
              Reporter:
              jamesdumay James Dumay
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: