Core: Preserve Relative Indexer Import Paths#34977
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPath Normalization Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Danger is currently failing only because the PR is missing maintainer-controlled labels:
The title format and |
|
Thanks for your PR, @AI-DEV-BOT. I am struggling to understand why it addresses the issue. In particular, #30612 was merged a while ago with a fix that ensures user importPaths are read and respected. It's our fault that the issue wasn't marked as closed, though. Sorry about that. Here are my questions:
We'll need answers to these questions, including human judgement, in order to progress the PR. Otherwise, I'll be closing the PR. Thanks! |
|
Thanks for taking the time to look at this, and for pointing me to #30612. I agree that #30612 already addressed the broader issue of making custom My reading is that this PR covers a narrower remaining case: when an indexer returns a non-virtual, already project-relative To answer your questions:
That said, I may have scoped this PR too broadly by linking it directly to #25554. I’m happy to retitle/reword it as a narrower follow-up to #30612 if that better reflects the intended change. |
|
@AI-DEV-BOT thanks for the analysis. You pointed out two risks:
Here's where things get interesting: our current code does not guarantee that both index authors and the core server make the same interpretation. Your PR does not guarantee that either, since it's impossible to guarantee! 😄 I think we must first enforce a single interpretation, and document it well in the Indexer API doc. Then, we must decide how we want to treat relative paths depends on which interpretation we enforce:
Do we agree on all this? If you reached the same conclusion as me, here's how I think you could help:
|
Fixes #25554.
Summary
importPathvalues as project-relative when they are not absolute paths or virtual modules.importPathhandling unchanged by normalizing it relative to the Storybook working directory../src/A.stories.jswhile indexing a different source file.Validation
git diff --checknode -e <path-normalization smoke check>confirmed the old logic produced../../../../../../src/A.stories.jswhile the new logic preserves./src/A.stories.jsI couldn't run the Vitest target locally because this checkout was missing Yarn's install state, and
yarn install --immutableis blocked in this sandbox by temp/cache filesystem limits (ENOSPConR:\TEMP, thenEPERMwriting Yarn's global cache).Manual testing