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(deps-dev): downgrade all Storybook breakage from dependabot et al #567

Merged
merged 17 commits into from
Aug 22, 2024

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jul 18, 2024

Fixes #469

Motivation

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

Modifications

devDeps

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
Screenshot 2024-07-18 at 3 54 13 PM

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 #432 (comment) 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 chore(storybook): update v1 syntax to use Storybook v6 #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

- go back to Storybook v6 addons, matching the `storybook` dep
  - the addons were partially auto-upgraded to breaking versions by dependabot
- go back to Webpack v4, matching Storybook v6
  - this was also auto-upgraded to a breaking version by dependabot
  - go back to `ts-loader` v8, matching Webpack v4
    - and this too had a breaking dependabot upgrade

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5 agilgur5 added type/dependencies Pull requests that update a dependency file javascript Pull requests that update Javascript code labels Jul 18, 2024
@agilgur5
Copy link
Contributor Author

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/[email protected]" has incorrect peer dependency "@storybook/addons@^3.2.16".

and @storybook/[email protected] 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
Copy link
Contributor Author

agilgur5 commented Jul 18, 2024

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 agilgur5 changed the title fix(deps): downgrade all breakage from dependabot et al fix(deps-dev): downgrade all breakage from dependabot et al Jul 18, 2024
@agilgur5
Copy link
Contributor Author

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

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
- the latter took some manual editing and resolutions until it got it, and now it reproduces the minimal variant

Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
- as per its archived docs
- tried to make the diff as small as possible

Signed-off-by: Anton Gilgur <[email protected]>
- same as downstream in Workflows, in order to code split properly, can't use the `src/` or `src/components/` imports as they have side effects (and therefore can't be tree-shaken)
  - instead have to do individual component imports to tree shake properly

- this seems to substantially improve performance from lethargic minutes of loading on first page load to seconds
  - I believe Storybook tries to code split things itself, so maybe it was making a giant bundle for each story file?

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Contributor Author

agilgur5 commented Jul 19, 2024

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 (storybookjs/storybook#4690 (comment), storybookjs/storybook#6364 (comment) 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 argoproj/argo-workflows#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
Copy link
Contributor Author

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)

- it was tested with and without the comment, to be specific
  - as well as several other ways

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Contributor Author

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

- per, well, my own SO answer: https://stackoverflow.com/a/78770854/3431180

- also remove old, no longer unused `@types/storybook*` deps
  - Storybook has types built-in now (and is written in TS, not sure if it wasn't before or something)

Signed-off-by: Anton Gilgur <[email protected]>
@agilgur5
Copy link
Contributor Author

agilgur5 commented Jul 19, 2024

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 storybookjs/storybook#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
Copy link
Contributor Author

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. 🙃

@agilgur5 agilgur5 marked this pull request as ready for review July 19, 2024 19:11
@agilgur5 agilgur5 changed the title fix(deps-dev): downgrade all breakage from dependabot et al fix(deps-dev): downgrade all Storybook breakage from dependabot et al Jul 19, 2024
@ashutosh16
Copy link

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
Copy link
Contributor Author

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

Copy link

@ashutosh16 ashutosh16 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Local test checks out. Thanks so much @agilgur5!

@crenshaw-dev crenshaw-dev merged commit 20b9acb into argoproj:master Aug 22, 2024
5 checks passed
@agilgur5 agilgur5 deleted the fix-downgrade-breakage branch August 22, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project setup fails with module build error
3 participants