Move Markdown components to subdirectory for modularity #19719
Conversation
guerler
added
kind/enhancement
area/UI-UX
kind/refactoring
There was a problem hiding this comment.
This is pretty good progress, but I feel, on some points, it is just a half-step in the right direction.
Instead of adding another isolated cache that doesn't respect types, we could leverage the existing Pinia stores more or improve them if needed.
Also, we should avoid using axios as much as we can while we have properly typed FastAPI routes that can be accessed using the GalaxyApi client.
Below are just some recommendations. I know using proper or stricter types is hard, but we are benefiting from it by avoiding a significant number of bugs in the future by investing in them now.
| const itemContent = ref<any>(null); | ||
| const loading = ref(true); | ||
| const messageText = ref<string | null>(null); | ||
| const messageVariant = ref<string | null>(null); | ||
|
|
||
| const itemName = computed(() => itemContent.value?.name || ""); | ||
| const itemUrl = computed(() => `${getAppRoot()}api/dataset_collections/${props.collectionId}`); | ||
| const downloadUrl = computed(() => `${getAppRoot()}api/dataset_collections/${props.collectionId}/download`); |
There was a problem hiding this comment.
We have FastAPI endpoints with typed responses for those requests. Please use them as much as you can instead of axios. Then itemContent will have the correct type instead of any.
Also, another recommendation: I know this was just converting the old code, but unless you really need to handle null cases in a meaningful way, I think it is better to write:
ref<string>(); instead of ref<string | null>(null); then you are only dealing with the defined/undefined case in your conditions.
There was a problem hiding this comment.
Thank you for all your comments—I completely agree. We should exclusively use stores and eliminate legacy Axios calls. However, I didn’t address this in this refactoring since my primary focus was restructuring the directories in this PR.
That said, another issue we might want to tackle is establishing a more consistent store interface. While we have many well-implemented stores, their interfaces are not entirely uniform.
That being said, I did replace the Axios call in this component with a schema-conformant call using Galaxy API. Additionally, I updated the ref to prevent null values.
client/src/components/Markdown/Sections/Elements/HistoryDatasetCollection/CollectionDisplay.vue
Show resolved
Hide resolved
client/src/components/Markdown/Sections/Elements/Workflow/WorkflowLicense.vue
Outdated
Show resolved
Hide resolved
| try { | ||
| const attributeName = ATTRIBUTES[props.name] || ""; | ||
| if (attributeName) { | ||
| const data = await fromCache(`datasets/${datasetId}`); |
There was a problem hiding this comment.
I'm not sure what benefit brings another generic cache when we have already a store for datasets and other entities, but I do see some drawbacks, like duplicated requests, duplicated memory and no type inference.
There was a problem hiding this comment.
I added a new text content store for datasets and removed the generic cache.
…s component instead of generic cache wrapper
…nstead of async/watch combo
…est for server calls
…quest explicitly asks for detailed data
guerler
force-pushed
the
revise_markdown_wrapper
branch
from
1a0ce71 to
3aed7b6
Compare
…isplay component (file_ext seems to be missing in pdfs?)
This PR modularizes Markdown components in preparation for supporting additional element types. Currently, we support only default/markdown and galaxy.
Additionally, this PR enhances Galaxy components by introducing reactivity, a necessary step for the upcoming component preview.
The PR also refactors components to render elements without requiring requests to the Pages API for dataset IDs. Instead, components now interact solely with their respective resource endpoints. To prevent redundant requests, a simple caching mechanism has been implemented.
These structural changes are essential for the upcoming markdown handling and editing features from #19226.
How to test the changes?
(Select all options that apply)
License