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

Indexer: Improve locating stories with specials chars in path #22110

Conversation

jankoritak
Copy link
Contributor

@jankoritak jankoritak commented Apr 15, 2023

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

  • 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"]

@jankoritak jankoritak changed the title 21636 fix locating stories with specials chars in path 21636: Fix locating stories with specials chars in path Apr 16, 2023
@jankoritak
Copy link
Contributor Author

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

@shilman
Copy link
Member

shilman commented Apr 29, 2023

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

@supryan
Copy link

supryan commented May 9, 2023

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

@ndelangen
Copy link
Member

@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.

@jankoritak
Copy link
Contributor Author

@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.

@ndelangen
Copy link
Member

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

@valentinpalkovic
Copy link
Contributor

This Pr's fix seems reasonable.

@shilman
Copy link
Member

shilman commented Nov 20, 2023

@ndelangen please take another look!

@ndelangen ndelangen changed the title 21636: Fix locating stories with specials chars in path Indexer: Improve locating stories with specials chars in path Nov 22, 2023
@ndelangen
Copy link
Member

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

@jankoritak
Copy link
Contributor Author

@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.

@ndelangen
Copy link
Member

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 ndelangen removed their assignment Nov 28, 2023
@JReinhold
Copy link
Contributor

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

@shilman shilman assigned ndelangen and unassigned shilman Jun 13, 2024
Copy link

nx-cloud bot commented Jun 18, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 95b7131. 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.

@ndelangen
Copy link
Member

I think i cleaned it up

@ndelangen ndelangen requested a review from tmeasday June 19, 2024 06:34
@valentinpalkovic valentinpalkovic merged commit 4f81812 into storybookjs:next Jul 2, 2024
50 checks passed
@valentinpalkovic valentinpalkovic added the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jul 4, 2024
@ghengeveld ghengeveld removed the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
9 participants