storybook icon indicating copy to clipboard operation
storybook copied to clipboard

Indexer: Improve locating stories with specials chars in path

Open jankoritak opened this issue 2 years ago • 16 comments

Closes #21636

What I did

  • Altered StoryIndexGenerator.ts not to treat this.options.workingDir as a glob. This allows the absolute part of the path to contain special glob characters without breaking the match.
  • The problem and proposed solution are described inside of the issue.
  • Note: As we're altering how we treat the local/absolute portion of the path, I'm unsure whether there is a way to test this.

How to test

  1. Insert a directory with a special character (e.g. Special (1)) in your path that contains a repository with a storybook.
  2. Run the storybook linked to the next branch.
  3. Verify that the stories are not loading (the storybook does not find any)
  4. Run the storybook linked to this branch.
  5. Verify that the stories are loaded.

Checklist

  • [x] Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • [x] Make sure to add/update documentation regarding your changes
  • [x] 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"]

jankoritak avatar Apr 15 '23 21:04 jankoritak

Hi @shilman, anything I can do to help push this PR toward a merge?

jankoritak avatar Apr 29 '23 07:04 jankoritak

@jankoritak Sorry for the delay. I'll make some time to review with @ndelangen this coming week.

shilman avatar Apr 29 '23 07:04 shilman

Thank you all for your hard work on this issue, this is currently impacting me. Hopefully a merge is around the corner!

supryan avatar May 09 '23 18:05 supryan

@jankoritak this PR I referenced got merged: https://github.com/storybookjs/storybook/pull/22186

Would you be able to apply the fixes from this PR on that part of the codebase as well? I'll update the branch for you.

ndelangen avatar May 16 '23 10:05 ndelangen

@jankoritak this PR I referenced got merged: #22186

Would you be able to apply the fixes from this PR on that part of the codebase as well? I'll update the branch for you.

@ndelangen Sure, I'll look into it. Thanks for the update.

jankoritak avatar May 16 '23 11:05 jankoritak

@JReinhold or @valentinpalkovic Is either of you able to determine if this is still something we need going forwards?

ndelangen avatar Jul 25 '23 13:07 ndelangen

This Pr's fix seems reasonable.

valentinpalkovic avatar Jul 25 '23 13:07 valentinpalkovic

@jankoritak could you resolve the merge conflicts?

ndelangen avatar Jul 26 '23 08:07 ndelangen

@jankoritak could you resolve the merge conflicts?

Hey @ndelangen, will do. I forgot about this PR, my bad!

jankoritak avatar Jul 26 '23 11:07 jankoritak

@ndelangen please take another look!

shilman avatar Nov 20 '23 10:11 shilman

@jankoritak I updated the branch, looks like CI found some problems, are you interested in fixing up this PR?

ndelangen avatar Nov 22 '23 13:11 ndelangen

@jankoritak I updated the branch, looks like CI found some problems, are you interested in fixing up this PR?

@ndelangen I am. However, I'm swamped at the moment. Would you guys be open to waiting for a couple more weeks till I find time to fine-tune the PR? In case this is a no-go for you, lemme know.

jankoritak avatar Nov 22 '23 13:11 jankoritak

That's not a problem, as a heads-up to you: The 7.6 release (should go out next week) will likely be the last 7.x release, and we'll be focussing on the 8.0 release soon. So there might be more merge conflict incoming. I'll help you when it comes to that though, just ping me for assistance.

ndelangen avatar Nov 22 '23 16:11 ndelangen

I think some merging has gone bad, as this now does const files = glob(...) twice right after each other.

JReinhold avatar May 07 '24 20:05 JReinhold

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 95b7131fc48d477046bb756f95dde2f07e818713. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

nx-cloud[bot] avatar Jun 18 '24 14:06 nx-cloud[bot]

I think i cleaned it up

ndelangen avatar Jun 18 '24 14:06 ndelangen