feat: adding gh changelog/releases to npmx#1233
feat: adding gh changelog/releases to npmx#1233WilcoSp wants to merge 134 commits intonpmx-dev:mainfrom
Conversation
…md -> html does need to change)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
excludes changelog releases due to needing api calls
app/components/Changelog/Card.vue
Outdated
| const cardId = computed(() => (release.publishedAt ? `date-${formattedDate.value}` : undefined)) | ||
|
|
||
| const navId = computed(() => `release-${slugify(release.title)}`) |
There was a problem hiding this comment.
Is there a chance that two cards will have the same id?
maybe sth like
cardId = ... `${navId}-card`There was a problem hiding this comment.
yes it can and even already does happen (nuxt), but it's on purpose with cardId because it's only to be a fallback when matching with navId didn't succeed
For releases npmx will first match with publish date + version and if not successful it'll then try to navigate to the first cardId that matches the publish date of the package version.
I'm going to change it that it'll only try to navigate to a date if a release for it is present.
| return resolved | ||
| } | ||
| return baseUrl | ||
| } |
There was a problem hiding this comment.
Do we really want to support custom changelogs? It feels so complicated and I'm not sure it worse it. @serhalp wdyt?
|
@WilcoSp amazing work, sorry I haven't looked at it in a while Most of the comments are trivial, and some of them could probably be moved to a separate issue. Could you also please add these pages to the robots.txt exceptions? |
|
@alexdln thanks for the review, I'll check later your comments and either implement your suggestions in this pr or in the next pr (will comment on thise moved to the next pr) with 'custom changelogs', I think you mean the custom markdown renderer for changelog. it's needed because readme & changelogs have different needs & contexts for example header links in changelog are only the icon in the header because vite has the version headers link to gh diff page for the both markdown renderers it's probably better in the next pr to look what can be shared with and without create functions to make maintaining both better. |
|
I've just applied most suggestions and marked them as resolved. the other I've left open due to a comment or I'll work on when back from the office tomorrow (19 march) at work I'm noticing that ungh.cc gh api keys are being exhausted rn, for this I'll add better error handling. the only issue I'm facing is that some packages have both releases and changelog.md in use (oxfmt), but some like msw only have a changelog.md to refer to releases, how can I best detect whether I can fallback to changelog.md and when not? |
added changelog to robots.txt
|
hey @alexdln I've just applied the last changes, when you want you can review again this pr. here are some of the larger changes
only the matching misses on branched versions for changelog i'll save for the next pr. for ungh.cc error handling has been added because today the api keys were rate limited a lot earlier today. |
|
(the storybook/chromatic ci breaking is my fault #2167) |
|
@ghostdevv can happen and at least in github it was clear why the ci wasn't fully successful. |
| export const ALLOWED_ATTR: Record<string, string[]> = { | ||
| '*': ['id'], // Allow id on all tags | ||
| 'a': ['href', 'title', 'target', 'rel'], | ||
| 'a': ['href', 'title', 'target', 'rel', 'content-none'], |
There was a problem hiding this comment.
It's not valid attribute. It probably should be data-content-none, but I would recommend to remove it. Looks like you can just check for all links in content (or in paragraphs) - but probably you can just add extra class from changelog page to Readme component and disable mark depends on this class
sth like
// package-changelog.vue
<Readme class="changelog-readme" />
...
.changelog-readme:deep(:is(h1, h2, h3, h4, h5, h6) a[href^='#']::after {
content: none;
}Or with extra prop in Readme (find better names please :))
// readme.vue
<div ... :class="!props.hideHeadingsMark && '.readme-heading-mark'">
...
.readme-heading-mark:deep(:is(h1, h2, h3, h4, h5, h6) a[href^='#']::after {
content: "__";
...
}
This is what I was wondering about in previous comments. Maybe we shouldn't support files, but only display what's in releases. In the case of files, an additional question is whether we should load a file from a specific version (and which hash is that?). And another big question is, maybe we should only show the changelog for the current version (incl. show tab only if there is block for this specific version)... I left another small comment. About other details, I think we can continue to improve iteratively - it seems we also could refactor the Markdown renderer and reuse it more with the main page In general lgtm, thanks 🤍 |
7 participants
This pr will add the possibility to view the changelog.md & releases from a package's github repo within npmx.
This will make it easier to see the changelogs while not needing to leave npmx and allowing quicker access.
This pr is the first pr of #501
Preview here, this will automatically update
Acknowledgements
While I was making this PR antfu also made this pr #1368 and here my comment from his pr
Also @ShroXd is currently working on adding changelogs to the version history page that was merged with #2025, we've both agreed to first independently work on our PRs and later after both are merged and we've received feedback & irl use we'll collaborate on the interaction between the two