appium icon indicating copy to clipboard operation
appium copied to clipboard

Published 20 hours ago • verified publisher icon appium

chore(appium,support): cache result of check for local appium

Open boneskull opened this issue 2 years ago • 3 comments

This is just a perf improvement. Anecdotally, this shaves 1-to-5 seconds off of an appium driver or appium plugin command, depending on system resources (lower end being CI VM's).

While poking around looking for a solution to #16916, I noticed that npm.list() in @appium/support was getting called twice. Each call is not fast; an npm child process must be forked. It ended up being two separate calls to hasAppiumDependency(). First, we need to resolve the Appium home dir, so we look for appium in npm ls output in the directory containing the closest package.json. Second, Appium's Manifest class needs to know this information, so it also calls hasAppiumDependency() which results in another npm ls. However, that is called with the value of Appium home--not necessarily the same directory as the first call.

It could be argued that the call in Manifest is actually a bug, since it shouldn't look at Appium home--it should look at the dir of the closest package.json. However, if the result of hasAppiumDependency(appiumHome) is true then this is the same directory. But semantically, it's incorrect. Anyway, it was weird. To avoid all this nonsense, I decided to just determine the answer earlier so we only need to do it once, and hand the result to the Manifest factory when loading extensions. If it's called again for whatever reason, the result will now be memoized (as will the call to find the closest package.json).

boneskull avatar May 11 '22 00:05 boneskull

this needs more time to bake

boneskull avatar May 11 '22 00:05 boneskull

Current dependencies on/for this PR:

  • 2.0
    • PR #16524 Graphite
      • PR #16594 Graphite
      • PR #16704 Graphite
      • PR #16883 Graphite
      • PR #16884 Graphite
      • PR #16885 Graphite
      • PR #16886 Graphite
      • PR #16887 Graphite
      • PR #16888 Graphite
      • PR #16891 Graphite
      • PR #16553 Graphite
      • PR #16907 Graphite
      • PR #16928 Graphite 👈
      • PR #16925 Graphite
      • PR #16937 Graphite
      • PR #16938 Graphite
      • PR #16940 Graphite
      • PR #16966 Graphite
      • PR #16992 Graphite
      • PR #16993 Graphite
      • PR #17045 Graphite
      • PR #17053 Graphite
      • PR #17055 Graphite
        • PR #16595 Graphite
        • PR #16892 Graphite
        • PR #16905 Graphite
        • PR #16902 Graphite
        • PR #16903 Graphite
        • PR #16904 Graphite
        • PR #16906 Graphite
        • PR #16908 Graphite
          • PR #16596 Graphite
            • PR #16597 Graphite

This comment was auto-generated by Graphite.

boneskull avatar May 12 '22 19:05 boneskull

ok, let us know when it is baked enough for review

jlipps avatar May 12 '22 20:05 jlipps

This is too out-of-sync

boneskull avatar Dec 07 '22 17:12 boneskull