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

Jump to bottom

remove glob-base dependency #15399

Merged
merged 4 commits into from Jul 23, 2021
Merged

remove glob-base dependency #15399

merged 4 commits into from Jul 23, 2021

Conversation

lkuechler
Copy link
Contributor

Issue: #15174

What I did

I removed the glob-base dependency to resolve the following security vulnerability #15174

How to test

No new tests should be needed.

Addition information

I had to reimplement some logic from glob-base. The returned glob is for example filled with the input path if no glob was detected.

@nx-cloud
Copy link

nx-cloud bot commented Jun 28, 2021 •
edited

Nx Cloud Report

CI ran the following commands for commit 852174e. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@shilman
Copy link
Member

shilman commented Jun 28, 2021

@lkuechler looks like the build is failing in examples:

ERR! => Failed to build the preview
ERR! Module not found: Error: Can't resolve 'src' in '/tmp/storybook/examples/react-ts'
ERR! ModuleNotFoundError: Module not found: Error: Can't resolve 'src' in '/tmp/storybook/examples/react-ts'
ERR!     at /tmp/storybook/node_modules/webpack/lib/Compilation.js:925:10
ERR!     at /tmp/storybook/node_modules/webpack/lib/ContextModuleFactory.js:143:23
ERR!     at /tmp/storybook/node_modules/neo-async/async.js:2830:7
ERR!     at /tmp/storybook/node_modules/neo-async/async.js:6877:13
ERR!     at /tmp/storybook/node_modules/webpack/lib/ContextModuleFactory.js:118:26
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/Resolver.js:213:14
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/Resolver.js:285:5
ERR!     at eval (eval at create (/tmp/storybook/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/UnsafeCachePlugin.js:44:7
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/Resolver.js:285:5
ERR!     at eval (eval at create (/tmp/storybook/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:13:1)
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/Resolver.js:285:5
ERR!     at eval (eval at create (/tmp/storybook/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:25:1)
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:67:43
ERR!     at /tmp/storybook/node_modules/enhanced-resolve/lib/Resolver.js:285:5
ERR!     at eval (eval at create (/tmp/storybook/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:41:1)
undefined

@shilman shilman added maintenance User-facing maintenance tasks security labels Jun 28, 2021
@lkuechler lkuechler force-pushed the remove-glob-base-dependency branch from 419268c to 8483ba9 Compare June 29, 2021 09:15
@lkuechler
Copy link
Contributor Author

hi @shilman,
thanks for the hint. I have now fixed the errors and the only pipelines that now fail also seem to fail in all other pull-requests :)

@powerchelle
Copy link

Hi @lkuechler, curious if this is still being actively pursued. Looks like last update was beginning of July. Do you have an ETA? Thanks in advance!

@shilman shilman added dependencies and removed maintenance User-facing maintenance tasks labels Jul 21, 2021
shilman
shilman requested changes Jul 21, 2021
View reviewed changes
lib/core-common/src/utils/to-require-context.ts Outdated
@@ -25,21 +25,33 @@ const detectBadGlob = (val: string) => {
const isObject = (val: Record<string, any>) =>
val != null && typeof val === 'object' && Array.isArray(val) === false;

const dirname = (pattern: string) => {
if (pattern.slice(-1) === '/') return pattern;
Copy link
Member

Choose a reason for hiding this comment

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

what if the user passes a directory but doesn't include a trailing slash? any chance you can provide a test case for this and for any other cases you considered in this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was behaviour was something that I saw in the glob-base dependency that was behaving a bit strange. I have now checked with the implementation in storybook on how it currently behaves and I believe that this extra logic did not make any difference. I have removed this now.

I think the current test are already very good but I added two more that focus on the folder pattern.

shilman
shilman approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-through @lkuechler -- looking great! 😍

@shilman shilman merged commit 9f676f7 into storybookjs:next Jul 23, 2021
This was referenced Jul 23, 2021
Closed
Merged
@shilman shilman added this to the 6.4 PRs milestone Jul 28, 2021
This was referenced Aug 3, 2021
Closed
Closed
shilman referenced this pull request Aug 10, 2021
@tmeasday
Add a to-importFn utility
0b77e47
@stof stof mentioned this pull request Oct 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shilman shilman shilman approved these changes

Assignees
No one assigned
Labels
dependencies security
Projects
None yet
Milestone
6.4 PRs
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lkuechler @shilman @powerchelle