search

argoproj / argo-ui

Argoproj shared React components
Apache License 2.0
220 stars 178 forks source link

fix(deps-dev): downgrade all Storybook breakage from dependabot et al #567

Closed agilgur5 closed 2 weeks ago

agilgur5 commented 1 month ago

Fixes #469

Motivation

Dependabot made lots of breaking changes prior to #537 that made the Storybook unrunnable

Modifications

devDeps

  • go back to Webpack v4, matching Storybook v6

  • remove deprecated, no longer used @types/storybook* deps

    • Storybook has types built-in now (and is written in TS, not sure if it wasn't before or something)

  • migrate from @dump247/storybook-state to useState hook in v1

    • as per its archived README
    • tried to make the diff as small as possible
    • in newer Storybook, this might make more sense to use Args like v2 does. that's for future work though, as I don't believe storiesOf syntax even supports that

    • others

    Verification

    Storybooks for v1 and v2 work now via yarn start and yarn start-v2:

    Screenshots: ![Screenshot 2024-07-18 at 3 51 31 PM](https://github.com/user-attachments/assets/984444f9-dbba-4fb8-ad1a-3f0da6e62603) ![Screenshot 2024-07-18 at 3 54 13 PM](https://github.com/user-attachments/assets/7c0cac1d-a06a-40bf-970c-fe4bfaee9330)

    Notes to Reviewers

    ~I could still not get yarn start to work. Got through a bunch of errors. I've put in too much time on attempting this, so I am giving up at this point since this repo is deprecated anyway per #453. Leaving this as a draft of my work for reference.~ EDIT: and apparently I was one change from it working apparently 😅

    ~I did also try upgrading everything to Storybook v7 or at least Storybook v6 with Webpack v5 in https://github.com/argoproj/argo-ui/pull/432#issuecomment-1879816619 and #568, and those had many more issues, so upgrading is not time well spent either.~ EDIT: now that this and #568 work, the upgrade might actually be doable. All the original dep mismatches etc might have been part of the issue.

    The CSS does not work for v1's Storybook and I couldn't get it to work after several attempts, but this is still much better than the previous unrunnable state and v2 at least works now.

    Future Work

    1. upgrade old Storybook v5 syntax and config -- see #568
    2. upgrade to Storybook v7 (and v8 now)
    3. switch v1 stories' React useState to Storybook Args, per above
    4. get CSS working for v1

    Postscript

    It seems likely that the Storybook build has not been working for years, since some of the reverts are years old and the incompatible changes made by humans would not have worked with the Storybook build. I am guessing it was only tested against Argo CD and the Storybook was unused.

    If that is the case, it may be worth deleting the Storybook entirely as it's been broken and unused for years and not worth maintaining. And has not been used in the dev process for at least an equivalent amount of time

    agilgur5 commented 1 month ago

    still erroring on one of Storybook's own imports

    or, actually that looks like the error originates from @dump247/storybook-state, which hasn't been updated in 5 years, i.e. long deprecated, and which has long been giving a warning during install about an incorrect peerDep version on @storybook/addons:

    warning " > @dump247/storybook-state@1.6.1" has incorrect peer dependency "@storybook/addons@^3.2.16".

    and @storybook/addons@3.2.16 is 6.5 years old...

    The upgrade path from it to useState is pretty easy, but I uh, have a feeling things won't start working after that either if it's been on an incorrect peer dep for that long 😕

    agilgur5 commented 1 month ago

    The upgrade path from it to useState is pretty easy, but I uh, have a feeling things won't start working after that either if it's been on an incorrect peer dep for that long 😕

    I commented some pieces out and, well, it did finally build, and then the Storybook window opened in the browser and completely froze.

    Suffice to say, I am pretty partial to just deleting the Storybook entirely now, so there is no confusion about its state or usability and to continue reducing the scope of this codebase until it is no longer used.

    agilgur5 commented 1 month ago

    it did finally build, and then the Storybook window opened in the browser and completely froze.

    Huh it did actually load and work fine, after I waited 3 minutes of my fans spinning quite loudly. So this actually might work... v2 started much quicker, so upgrading from storiesOf etc syntax in #568 might improve that poor performance

    agilgur5 commented 1 month ago

    Huh it did actually load and work fine, after I waited 3 minutes of my fans spinning quite loudly.

    despite this long load time, the CSS also did not load, which makes the v1 Storybook not very useful. the v2 one loads fine. I tried a bunch of suggestions from Storybook issues (https://github.com/storybookjs/storybook/issues/4690#issuecomment-435909433, https://github.com/storybookjs/storybook/issues/6364#issuecomment-584198083 etc) and not one of them worked 😕 I saw that there were some in-line styles added in the component iframe, but they were all <style>undefined</style>, which definitely suggests some error is happening and not getting logged. Also building a static Storybook (i.e. w/o dev server) had the exact same issues (no CSS but long load time).

    This PR is still useful as-is though, so I guess we'll leave that for another PR or maybe the upgrade to Storybook v7 or v8 or Webpack v5 will resolve this.

    v2 started much quicker, so upgrading from storiesOf etc syntax in #568 might improve that poor performance

    The biggest difference seemed to not be the upgrade from storiesOf syntax but actually individual component imports per https://github.com/argoproj/argo-workflows/pull/12158. Then it seemed that importing the global CSS was the main cause of it slowing down, despite the import doing nothing per above 🫠 .

    agilgur5 commented 1 month ago

    Huh, CI has a failure that I don't have locally:

    ERR! TypeError: Cannot read properties of null (reading 'babel')
ERR!     at _callee$ (/home/runner/work/argo-ui/argo-ui/node_modules/@storybook/telemetry/dist/cjs/storybook-metadata.js:202:52)
ERR!     at tryCatch (/home/runner/work/argo-ui/argo-ui/node_modules/regenerator-runtime/runtime.js:64:40)
ERR!     at Generator.invoke (/home/runner/work/argo-ui/argo-ui/node_modules/regenerator-runtime/runtime.js:299:22)
ERR!     at Generator.next (/home/runner/work/argo-ui/argo-ui/node_modules/regenerator-runtime/runtime.js:124:21)
ERR!     at asyncGeneratorStep (/home/runner/work/argo-ui/argo-ui/node_modules/@storybook/telemetry/dist/cjs/storybook-metadata.js:68:103)
ERR!     at _next (/home/runner/work/argo-ui/argo-ui/node_modules/@storybook/telemetry/dist/cjs/storybook-metadata.js:70:194)
ERR!     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    agilgur5 commented 1 month ago

    Huh, CI has a failure that I don't have locally:

    Ah it only happens on build, but not on start; that I can repro. Also same error in this unanswered SO question. The error traces back to this line in Storybook code where I'm guessing mainConfig refers to main.js, which does not exist in v1. Notably #568 does not have this error, and it moves to using main.js

    agilgur5 commented 1 month ago

    The error traces back to this line in Storybook code where I'm guessing mainConfig refers to main.js, which does not exist in v1.

    I tried downgrading to Storybook 6.4.22 (latest of 6.4.x) which does not have @storybook/telemetry (the line above was introduced in https://github.com/storybookjs/storybook/pull/18046), but then started getting a typecheck error on react-router for some reason 😕 Then upgraded to 6.5.0-alpha.64 and the Storybook failed to load with some errors in the browser console. Then upgraded to 6.5.0-beta.1 and had the same issue, but once I reset my node_modules, it went away (that's happened twice now; stale cache with Yarn or something?)

    That does also suggest that without all the other errors, #244 would also have broken the build 😕

    Also same error in this unanswered SO question.

    I answered it myself 😅

    agilgur5 commented 1 month ago

    I've put in too much time on attempting this, so I am giving up at this point since this repo is deprecated anyway per #453.

    Suffice to say, I am pretty partial to just deleting the Storybook entirely now, so there is no confusion about its state or usability and to continue reducing the scope of this codebase until it is no longer used.

    For better or for worse, I had a pretty bad migraine all of yesterday (and a bit of the night before and this morning), so I worked on this yesterday as it was something fairly straightforward that didn't require a lot of brainpower, i.e. trial, wait, error, trial, wait, error, repeat until working with right versions, config, etc. Tedious af, but not really "difficult" per se. 🙃

    ashutosh16 commented 2 weeks ago

    thank you @agilgur5 for putting the effort. I agree with you comments and I think this is good starting point to get the storybook fix. I'm not sure if the dependent bot will update the dependencies and break again.

    agilgur5 commented 2 weeks ago

    I'm not sure if the dependent bot will update the dependencies and break again.

    It won't, it now only runs for security fixes: #537