-
-
Notifications
You must be signed in to change notification settings - Fork 103
feat: allow filePath
to be null
on a route added in an editable tree
#607
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
base: main
Are you sure you want to change the base?
Conversation
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 61.72% 60.90% -0.82%
==========================================
Files 32 36 +4
Lines 3135 3379 +244
Branches 580 619 +39
==========================================
+ Hits 1935 2058 +123
- Misses 1194 1314 +120
- Partials 6 7 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 know the comment is a bit weird. I didn't find any practical case where this was useful when inserting an already parsed node. I couldn't find any usage of this in Nuxt code. Where is it?
Hmm, it's been a while since I last looked at this. I think it's these two lines, where
Here you can see that I am not entirely sure, but maybe Nuxt supports virtual pages that don't really correspond to files? If that's true and this PR is to be merged, then we need to adjust the lines mentioned above to fall back to |
Here is an example in a test file, where no Not sure if this test case is actually realistic, but Nuxt does seem to support some form of file-less pages. |
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.
Thanks for this. I will come back to it after merging other things like relative paths insertions. I'm not 100% sure about it yet but I think it makes sense in the context of the editable tree where a node can be created to have children afterwards
Co-authored-by: Eduardo San Martin Morote <posva@users.noreply.github.com>
WalkthroughThe changes update method signatures in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)src/core/extendRoutes.ts (1)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
It seems that, when we only allow
filePath
to benull
on a route added in an editable tree, that shouldn't cause any problems.In Nuxt, we don't use file scanning and add routes by modifying the
EditableTreeNode
of the root page in thebeforeWriteFiles
callback. ANuxtPage
can theoretically not have a correspondingfile
, although I'm not sure what the use case here is exactly. So currently, where we runparent.insert(path, page.file)
,page.file
can beundefined
and we currently use a@ts-expect-error
before that line to suppress TS errors.If this PR is safe to merge, we can remove that
@ts-expect-error
.Summary by CodeRabbit
New Features
Tests