-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix: Module graph in workspace of UI package #3636
Conversation
Signed-off-by: GurkiranSingh <[email protected]>
Run & review this pull request in StackBlitz Codeflow. |
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 don't think this is a good solution. Module Graph should show module from the same workspace as the test file, not a random one
@sheremet-va Please, correct me if I am wrong, but isn't this code will show module from the same workspace as the test file, as I am showing the module based on the id of the file. const workspaceModules = workspaceProject?.ctx.getModuleProjects(id)
let workspaceMod: ModuleNode | undefined
if (workspaceModules && workspaceModules.length === 1) {
const { server, browser } = workspaceModules[0]
const mod = server.moduleGraph.getModuleById(id) || browser?.moduleGraph.getModuleById(id)
workspaceMod = mod
} |
It's possible to have the same test file in different workspace projects and process them differently. There is an example in test/workspaces with
|
Ah okay, Thanks, Do you have suggestions about how this should be resolved then ? |
Show a button with all workspaces (if there are more than one) to choose from on the module graph page. |
Or! Which is probably better for now - display workspace name near the test file in the spec list. I think test files are now duplicated without the name there. Then you can just pass workspace name to the module graph |
…e workspace module fetch logic as per Vladimir suggestions Signed-off-by: GurkiranSingh <[email protected]>
✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.
|
@@ -23,7 +23,7 @@ const duration = computed(() => { | |||
> | |||
<StatusIcon :task="task" mr-2 /> | |||
<div flex items-end gap-2 :text="task?.result?.state === 'fail' ? 'red-500' : ''"> | |||
<span text-sm truncate font-light>{{ task.name }}</span> | |||
<span text-sm truncate font-light>{{ `${(task.type === "suite" && task.projectName) ? `${task.projectName}/` : ''}${task.name}` }}</span> |
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 don't like how it's shown. In CLI it's marked as [${name}]
and has a color
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.
@sheremet-va Like this ?
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.
yes
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.
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.
@sheremet-va How about we if use a bit of blackish-shade with yellow - #7c7c05
, green - #025402
and blue - rgb(2 2 131)
color ?
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.
happy-dom still looks unreadable to me
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.
@sheremet-va How's this ?
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.
Looks readable. Is it only for light mode? Won't be readable in dark mode
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.
Change colors based on theme, use isDark
from here https://github.com/vitest-dev/vitest/blob/main/packages/ui/client/composables/dark.ts
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.
Yes, the colors in this image will only be for light mode - #3636 (comment)
Signed-off-by: GurkiranSingh <[email protected]>
Signed-off-by: GurkiranSingh <[email protected]>
workspaceMod = mod | ||
} | ||
|
||
await get(ctx.server.moduleGraph.getModuleById(id) || workspaceMod) |
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.
Shouldn't you fallback to ctx.server.moduleGraph
only if you didn't find the workspaceMod
? This line looks suspicious to me
|
||
const workspaceProject = ctx.projects.find(project => project.getName() === workspaceName) | ||
const workspaceModules = workspaceProject?.ctx.getModuleProjects(id) | ||
const selectedModule = workspaceModules?.find(module => module.getName() === workspaceName) |
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.
You do the same thing you did on #L33. Why?
You need to find the project by name and fallback to ctx.getCoreWorkspaceProject().server || ctx.server
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.
@sheremet-va getCoreWorkspaceProject()
is coming as null
for test/workspaces
folder.
Upon investigating, I have found that the conditons that executes the createCoreWorkspace
function in packages\vitest\src\node\core.ts
aren't satisfying
You do the same thing you did on #L33. Why?
This might be hacky, but I have found only this way where we get the workspaceMod
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.
getCoreWorkspaceProject()
is coming asnull
fortest/workspaces
folder.
Yes, it will be null if there is no core project. This is why I added || ctx.server
. Core project is not created if main config file is not used by a workspace (so, when there is a global config and project configs and workspace specifies only project configs without manually using core project)
Core workspace project should be used AS THE LAST RESORT. Regular projects should be used if they are found by name.
This might be hacky, but I have found only this way where we get the
workspaceMod
Then you must be doing something wrong?
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.
Workspace project is just a project with the same name. If there is no such projects, just fallback to context which is always defined
await get(ctx.server.moduleGraph.getModuleById(id)) | ||
|
||
const workspaceProject = ctx.projects.find(project => project.getName() === workspaceName) | ||
const workspaceModules = workspaceProject?.ctx.getModuleProjects(id) |
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.
This code can be removed, you already have access to ctx
Any update on this? I would be happy to create another PR to get this fix merged. |
Signed-off-by: GurkiranSingh <[email protected]>
Feel free to open a separate PR. |
This is fixed in |
fix: #3470