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: add overrides to pin minimum versions #29723

Closed
wants to merge 2 commits into from

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Jul 26, 2024

SUMMARY

Fixing a few breaking npm package issues. One of which is puppeteer based on puppeteer/puppeteer#12094

It also looks like we need lockfile version 3 for npm 9+. Ref: #27198

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:
puppeteer is being imported via eyes-storybook which is at it's current latest version

puppeteer@21.11.0 dev
node_modules/@applitools/eyes-storybook/node_modules/puppeteer
  puppeteer@"21.11.0" from @applitools/eyes-storybook@3.50.7
  node_modules/@applitools/eyes-storybook
    dev @applitools/eyes-storybook@"^3.50.7" from the root project

npm warn deprecated puppeteer@21.11.0: < 22.8.2 is no longer supported

after:

(superset) elizabeththompson@me superset-frontend % npm explain puppeteer
puppeteer@22.14.0 dev overridden
node_modules/puppeteer
  overridden puppeteer@"^22.4.1" (was "21.11.0") from @applitools/eyes-storybook@3.50.7
  node_modules/@applitools/eyes-storybook
    dev @applitools/eyes-storybook@"^3.49.0" from the root project
  peer overridden puppeteer@"^22.4.1" (was ">=5.3.0") from @applitools/spec-driver-puppeteer@1.4.11
  node_modules/@applitools/spec-driver-puppeteer
    @applitools/spec-driver-puppeteer@"1.4.11" from @applitools/eyes-storybook@3.50.7
    node_modules/@applitools/eyes-storybook
      dev @applitools/eyes-storybook@"^3.49.0" from the root project

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@eschutho
Copy link
Member Author

I deleted my package-lock.json file, ran npm update and then npm install again to get around this issue: #29658

@mistercrunch
Copy link
Member

I deleted my package-lock.json file, ran npm update and then npm install again

I think by doing that you're effectively upgrading all libs within the specified range in package.json which leads to 38,063 additions, 98,230 deletions not shown because the diff is too large in package-lock, which seems like you're entering a world of trouble.

Better to get dependabot to trigger dozens of PRs, and making it clear which bump creates what issue.

Though it'd be super nice to bump all at once, I'm just thinking it might be a super intricate dead end.

@rusackas
Copy link
Member

I guess there's a lot going on in this PR, so we can tease apart a few topics, even if this can't be teased apart into multiple topics (or even PRs), namely:

  1. Package bumps: Looks like there are really only 3 bumps happening in the package.json file, which is cool. I'm not sure which of those led to the rest of the changes, or if they can be done independently to see what breaks where.

  2. Lockfile bump: I wasn't sure about the v3 package lock file yet. From what I understand, npm v9 and v10 support package lock v2 still. While we ought to modernize, I was basically punting on that part. It might be worth opening one PR that does only this, to see how big the diff is. It'd probably be best to skip the npm upgrade step and just rm package-lock.json then npm install

  3. ES Lint It looks like there are a ton of linting problems... I'm not sure how many stem from the upgrades, but it looks like there are also some rule flips going on in here, too, with the rule disabled in individual files. That might also be worth tackling/discussing on a separate PR if it helps, us not have to clean it up in this PR.

@mistercrunch
Copy link
Member

mistercrunch commented Jul 31, 2024

ES Lint It looks like there are a ton of linting problems

I'm guessing eslint got updated to a newer version while deleting/regenerating package-lock.json which really should be isolated in its own PR. Just that one eslint bump could be a significant undertaking.

Side note - I absolutely love ruff's --add-noqa flag where it add a # noqa flag to all the lines that don't comply to the rules in place. It allows for easily implementing/upgrading, while preventing the lint to accumulate from that point onward. There's also a --ignore-noqa flag to report all the lint issues (even the ones that have had a # noqa) for when you want to work on applying new rules to old code. I asked GPT about whether eslint had similar features but it didn't seem like it did.

@eschutho
Copy link
Member Author

eschutho commented Aug 1, 2024

Ok, I decided to start from scratch since the dependency with the optionals is fixed separately, and this seems to have worked: #29805

@eschutho eschutho closed this Aug 1, 2024
@rusackas rusackas deleted the elizabeth/pin-npm-versions branch September 27, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants