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

Update glob to v10.0.0 #22171

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Update glob to v10.0.0 #22171

merged 4 commits into from
Apr 28, 2023

Conversation

NotWoods
Copy link
Contributor

What I did

Updated the glob library to v10, which includes a promise-based API and a min version of Node 16. Also removed the no longer needed glob-promise library.

How to test

This updates the logic for listing stories in a vite project. To test:

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Open Storybook in your browser
  3. Check that all expected stories are displayed.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@socket-security
Copy link

socket-security bot commented Apr 19, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

@ndelangen
Copy link
Member

@NotWoods I'll update this branch with the base-branch.

Can you investigate the unit-test failures?

@NotWoods
Copy link
Contributor Author

Which check does the unit test failures appear under?

@ndelangen
Copy link
Member

ah never mind, might have been inconsistent. I'll get this updated, and merged.

@blowsie
Copy link

blowsie commented May 9, 2023

This change broke some things...
Is there an open issue for it or just the PR?

@zoontek
Copy link

zoontek commented Jul 21, 2023

Having glob@^10.0.0 as dependency broke a lot of CI CLI tools, including eslint: eslint/eslint#17215 (reply in thread)

The last safe version of glob is the 10.1.0 (as 10.2.0 starts to rely on the package jackspeak) (from this commit isaacs/node-glob@94a36ce)

A solution could be to switch to fast-glob. I can do the PR if required.

EDIT: The author of glob (and of npm) said it's an issue with yarn and say the issue is to switch to npm: isaacs/node-glob@94a36ce#commitcomment-122320956

Not sure that's a realistic solution.

@ndelangen
Copy link
Member

@JReinhold Didn't your PR address this problem?

@koni1234
Copy link

Having glob@^10.0.0 as dependency broke a lot of CI CLI tools, including eslint: eslint/eslint#17215 (reply in thread)

The last safe version of glob is the 10.1.0 (as 10.2.0 starts to rely on the package jackspeak) (from this commit isaacs/node-glob@94a36ce)

A solution could be to switch to fast-glob. I can do the PR if required.

EDIT: The author of glob (and of npm) said it's an issue with yarn and say the issue is to switch to npm: isaacs/node-glob@94a36ce#commitcomment-122320956

Not sure that's a realistic solution.

I agree, with storybook 7.3 and 7.4 using yarn i have the same problems ... eslint and webpack scripts are broken

@shilman shilman mentioned this pull request Aug 30, 2023
4 tasks
@shilman
Copy link
Member

shilman commented Aug 30, 2023

@zoontek @koni1234 WDYT about using yarn resolutions to pin the version to 10.1.0? I created a PR to try to pin it in SB but @tmeasday mentioned that we have indirect dependencies on it that we can't control, so I closed it. #24002

@zoontek I'd be up for a fast-glob replacement. If you could create a PR for that, we could test it out and see whether it works well.

@tmeasday
Copy link
Member

Replacing with fast-glob doesn't help either due to the indirect dependencies. Unless we also fork all the packages that depend on glob themselves

@shilman
Copy link
Member

shilman commented Aug 30, 2023

@yannbf is it possible to catch this error & tell users to pin their dependencies? if so, can you squeeze this into your error handling project?

@IanVS
Copy link
Member

IanVS commented Aug 30, 2023

FWIW, I took a stab at replacing globby with fdir a while back, for compatibility and performance reasons. It looks like fdir has been improved further since then, and I plan to take another shot at it, though it sounds like that's insufficient due to transitive deps.

@koni1234
Copy link

@zoontek @koni1234 WDYT about using yarn resolutions to pin the version to 10.1.0? I created a PR to try to pin it in SB but @tmeasday mentioned that we have indirect dependencies on it that we can't control, so I closed it. #24002

@zoontek I'd be up for a fast-glob replacement. If you could create a PR for that, we could test it out and see whether it works well.

yes, in order to temporary solve this issue I added these dependencies resolutions in the package.json

"resolutions": {
"strip-ansi": "^6.0.1",
"string-width": "^4.2.2",
"wrap-ansi": "^7.0.0"
},

@tmeasday
Copy link
Member

@koni1234 you only need to pin jackspeak as documented: #22431 (comment)

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.

9 participants