-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
unique file entries in integrity file #6704
base: master
Are you sure you want to change the base?
Conversation
It seems good, but for posterity can you detail why duplicate entries can even occur? The old code was iterating over |
|
@arcanis @rally25rs So, I've reached the root cause of this issue. Which, lead me to a better solution for this. Basically, |
.filter(Boolean) | ||
// $FlowFixMe | ||
.map(({loc}) => path.join(loc, constants.NODE_MODULES_FOLDER)), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer to keep using the for...of
style that the previous code was using. This particular chaining of the array methods makes the code more confusing imo and doesn't help Flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arcanis So, im happy to restore the sequential awaits. I just thought this would be preferred in terms of perf. Flow barfs on comparing custom types with Object.values() entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no await here, right?
if (workspaceLayout) { | ||
for (const workspaceName of Object.keys(workspaceLayout.workspaces)) { | ||
const loc = workspaceLayout.workspaces[workspaceName].loc; | ||
const folders = await Promise.all(locations.map(async loc => (await fs.exists(loc)) && loc)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this lambda return loc && (await fs.exists(loc))
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arcanis you would be correct, if it were using filter instead of map
const locations = []; | ||
|
||
if (this.config.modulesFolder) { | ||
locations.push(this.config.modulesFolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've removed this part of the code without replacement - won't that break the modulesFolder
settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arcanis this was actually causing the issue, since these dirs are included already.
For ex, i added the following log:
console.log(
this.config.modulesFolder,
path.join(this.config.lockfileFolder, constants.NODE_MODULES_FOLDER),
Object.values(workspaceLayout.workspaces)
.filter(Boolean).map(({loc}) => loc)
);
which yields:
undefined '/Users/myuser/dev/repos/my-repo/node_modules' [ '/Users/myuser/dev/repos/my-repo/components',
'/Users/myuser/dev/repos/my-repo/contexts',
'/Users/myuser/dev/repos/my-repo/modules',
'/Users/myuser/dev/repos/my-repo/pages',
'/Users/myuser/dev/repos/my-repo/packages/browser-dev-tools',
'/Users/myuser/dev/repos/my-repo/packages/dal',
'/Users/myuser/dev/repos/my-repo/packages/dev-certs',
'/Users/myuser/dev/repos/my-repo/packages/env',
'/Users/myuser/dev/repos/my-repo/packages/koa-auth-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-error-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-transform-multistats-assets',
'/Users/myuser/dev/repos/my-repo/packages/koa-wc-headers-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-wc-health-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-wc-logging-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-wc-middleware',
'/Users/myuser/dev/repos/my-repo/packages/koa-webpack-dev-middleware',
'/Users/myuser/dev/repos/my-repo/packages/logger',
'/Users/myuser/dev/repos/my-repo/packages/module-interface',
'/Users/myuser/dev/repos/my-repo/packages/react-render-to-stream',
'/Users/myuser/dev/repos/my-repo/packages/redux-create-store',
'/Users/myuser/dev/repos/my-repo/packages/server',
'/Users/myuser/dev/repos/my-repo/services/akamai-headers',
'/Users/myuser/dev/repos/my-repo/services/hello-world',
'/Users/myuser/dev/repos/my-repo/services/redux-dal',
'/Users/myuser/dev/repos/my-repo/services/storybook',
'/Users/myuser/dev/repos/my-repo/tools/cli',
'/Users/myuser/dev/repos/my-repo/tools/lib-src-resolver',
'/Users/myuser/dev/repos/my-repo/tools/platform-cache',
'/Users/myuser/dev/repos/my-repo/tools/platform-config',
'/Users/myuser/dev/repos/my-repo/tools/platform-core',
'/Users/myuser/dev/repos/my-repo/tools/progress-webpack-plugin',
'/Users/myuser/dev/repos/my-repo/tools/storybook-helpers',
'/Users/myuser/dev/repos/my-repo/tools/webpack-chunk-assets-plugin',
'/Users/myuser/dev/repos/my-repo' ]
d3163ac
to
2ca9cce
Compare
@hulkish are you still interested in getting this PR merged? I came across this problem too as detailed in celo-org/celo-monorepo#2798 |
Summary
Fixes #6703
Test plan
After installing dependencies with this modified yarn, run:
You should only see a 1 next to each entry.