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

Core: Fix static dirs targeting same destination #19064

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Aug 31, 2022

Issue: #16732

What I did

The previous implementation caused a race-condition if two assets target the same destination

  staticDirs: [
    '../src/assets',
    // reproduction of https://github.com/storybookjs/storybook/issues/16732
    { from: '../src/assets/a', to: 'assets' },
    { from: '../src/assets/b', to: 'assets/b' },
  ],

With this change, one staticDir after another gets copied without conflicting in a race condition

How to test

  • Checkout this branch
  • Revert my "fix" (there is a separate commit for the reproduction)
  • Go to "examples/angular-cli" and run yarn build-storybook

The error in the mentioned issue should appear (mkdir file already exists)

@kroeder kroeder added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Aug 31, 2022
@kroeder kroeder self-assigned this Aug 31, 2022
@kroeder kroeder changed the base branch from master to main August 31, 2022 10:09
@kroeder kroeder removed their assignment Aug 31, 2022
@kroeder
Copy link
Member Author

kroeder commented Aug 31, 2022

@ndelangen eslint is very unhappy the way I solved this

Though. I don't see the point here. The function above uses

await Promise.all( staticDirs.map( async (dir) => ... ));

which creates a map of promises and awaits them (technically also a loop with awaits in it?)
I even tried to use Promise.all but the error was not solved (which I also did not understand 🤷‍♂️ )

I targeted "main" because of the restructuring in "next". I was not able to build the angular-cli example

@kroeder kroeder force-pushed the fix-staticDirs-file-already-exists-16732 branch from 06e0152 to 63e28f3 Compare August 31, 2022 11:17
@kroeder kroeder changed the base branch from main to next August 31, 2022 11:28
@kroeder kroeder changed the base branch from next to main August 31, 2022 11:32
Kai Röder added 2 commits August 31, 2022 13:33
Using forEach with await in the callback does not work.
Changing it to a for-of loop handles the async staticDirs copy task
correctly without creating a race-condition on writing
to the same target directory.

Fixes #16732
@kroeder kroeder force-pushed the fix-staticDirs-file-already-exists-16732 branch from 63e28f3 to ed495aa Compare August 31, 2022 11:35
@kroeder kroeder changed the base branch from main to next August 31, 2022 11:35
@shilman shilman changed the title Fix static dirs file already exists Core: Fix static dirs targeting same destination Sep 1, 2022
@shilman shilman added the core label Sep 1, 2022
@ndelangen ndelangen self-assigned this Sep 8, 2022
@ndelangen ndelangen merged commit 50617bc into next Sep 8, 2022
@ndelangen ndelangen deleted the fix-staticDirs-file-already-exists-16732 branch September 8, 2022 15:11
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug core patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants