-
-
Notifications
You must be signed in to change notification settings - Fork 102
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
=======================================
Coverage 61.72% 61.72%
=======================================
Files 32 32
Lines 3135 3135
Branches 580 580
=======================================
Hits 1935 1935
Misses 1194 1194
Partials 6 6 ☔ 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
const isComponent = true | ||
insertParsedPath(path: string, filePath: string | null = path): TreeNode { | ||
// Allow null filePath to be handled | ||
const isComponent = filePath !== null |
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.
const isComponent = filePath !== null | |
const isComponent = filePath != null |
// TODO: allow null filePath? | ||
const isComponent = true | ||
insertParsedPath(path: string, filePath: string | null = path): TreeNode { | ||
// Allow null filePath to be handled |
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.
// Allow null filePath to be handled |
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
.