Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
joaopedrodcf
left a comment
There was a problem hiding this comment.
Hope you guys don't mind me trying to review the PR 🙏
| <code>{{ replacement.replacementModule }}</code> | ||
| </template> | ||
| <template #community> | ||
| <a |
There was a problem hiding this comment.
I think we should use NuxtLink but what do you think ?
There was a problem hiding this comment.
I think NuxtLink is for internal links?
There was a problem hiding this comment.
It also works for external links as far as I know
https://nuxt.com/docs/4.x/api/components/nuxt-link#external-routing
There was a problem hiding this comment.
I've kept <a> as that what was there before
| <PackageReplacement | ||
| v-if="moduleReplacement" | ||
| :mapping="moduleReplacement.mapping" | ||
| :replacement="moduleReplacement.replacement" | ||
| /> |
There was a problem hiding this comment.
I think we always have to options parent fetchs and decides if renders or not or we put that logic inside the component and inside we have a v-if to not render html etc
Maybe you can even have a component packageReplacementIntegration that is responsible to fetch the replacement and the rendering the visual part
| }) | ||
|
|
||
| const replacementDescription = useMarkdown(() => ({ | ||
| text: (props.replacement as { description?: string }).description ?? '', |
There was a problem hiding this comment.
we should probably just have a shared function in this repo that computes the description:
function getReplacementDescription(replacement) {
switch (replacement.type) {
case 'documented':
return undefined;
default:
return replacement.description;
}
}There was a problem hiding this comment.
Should that be in module-replacements itself since it might be useful for other consumers too or just here?
There was a problem hiding this comment.
There was a problem hiding this comment.
should this use the getReplacementDescription fn you added here?
| try { | ||
| const replacement = await $fetch<ModuleReplacement | null>(`/api/replacements/${name}`) | ||
| const response = await $fetch<{ | ||
| mapping: ModuleReplacementMapping |
There was a problem hiding this comment.
@danielroe iirc you said these types are inferred based on the route. is that true? does this not infer them because of the interpolation?
| replacements: readonly(replacements), | ||
| noDepSuggestions: readonly(noDepSuggestions), | ||
| infoSuggestions: readonly(infoSuggestions), | ||
| replacements, |
There was a problem hiding this comment.
are these still readonly? was that dropped on purpose?
There was a problem hiding this comment.
as far as I can see seems by mistake,
There was a problem hiding this comment.
It was intentional in e16308c because otherwise https://github.com/npmx-dev/npmx.dev/actions/runs/23078681163/job/67043682503 was failing
There was a problem hiding this comment.
it should probably be fixed if possible. i.e. keep readonly and figure out why the type error happens
| /** | ||
| * Replacement types that suggest "no dependency" (can be replaced with native code or inline). | ||
| */ | ||
| const NO_DEP_REPLACEMENT_TYPES = ['native', 'simple'] as const |
There was a problem hiding this comment.
| const NO_DEP_REPLACEMENT_TYPES = ['native', 'simple', 'removal'] as const |
should this be in here now?
| }) | ||
|
|
||
| const replacementDescription = useMarkdown(() => ({ | ||
| text: (props.replacement as { description?: string }).description ?? '', |
There was a problem hiding this comment.
should this use the getReplacementDescription fn you added here?
| const replacementMap = new Map<string, ModuleReplacement>( | ||
| all.moduleReplacements.map(r => [r.moduleName, r]), | ||
| export default defineEventHandler( | ||
| (event): { mapping: ModuleReplacementMapping; replacement: ModuleReplacement } | null => { |
There was a problem hiding this comment.
is it intentional we only return the first replacement - now there can be multiple right?



Module replacements v3
Description
Update
module-replacementsto v3Update
Compare/ReplacementSuggestion.vue,Package/Replacement.vue,npm/useReplacementDependencies.ts,useCompareReplacements.ts,useModuleReplacement.ts,package/[[org]]/[name].vue,replacements/[...pkg].get.tsaccordinglyRemove
"MDN"from localesUpdate existing tests:
a11y.spec.ts,composables/use-replacement-dependencies.spec.ts,composables/use-compare-replacements.spec.tsAdd new test fixture
npm-registry/packuments/strip-ansi.jsonAdd new tests:
e2e/compare-replacements.spec.ts,e2e/package-replacements.spec.tsScreenshots
Simple
Native
Documented
Removal
Compare page