feat: add a warning when the package license changes#2188
feat: add a warning when the package license changes#2188aisiklar wants to merge 12 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change introduces a license-change detection and notification system for package versions. A new composable fetches npm registry metadata for a given package, extracts and chronologically sorts all versions, then compares licenses between consecutive versions to identify transitions. When changes are detected, they are displayed via a warning component that renders an amber alert panel with formatted change details. Supporting internationalization strings have been added to the schema and locale files in English and Turkish. Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ghostdevv
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7c1c88e-8b86-4447-9fdb-bfbc10c1ff70
📒 Files selected for processing (6)
app/components/LicenseDisplay.vueapp/composables/useLicenseChanges.tsapp/pages/package/[[org]]/[name].vuei18n/locales/en.jsoni18n/locales/tr-TR.jsoni18n/schema.json
hi @ghostdevv |
|
Awesome! I'll mark this as draft for now, and you can mark it as ready when you want me to take another look! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…sorting versions to date of release
|
Hi @ghostdevv |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 274b5e5e-8053-43ca-946d-f111b04b96f3
📒 Files selected for processing (5)
app/components/LicenseDisplay.vueapp/composables/useLicenseChanges.tsi18n/locales/en.jsoni18n/locales/tr-TR.jsoni18n/schema.json
✅ Files skipped from review due to trivial changes (2)
- i18n/locales/en.json
- i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- i18n/locales/tr-TR.json
- app/composables/useLicenseChanges.ts
app/components/LicenseDisplay.vue
Outdated
| aria-hidden="true" | ||
| /> | ||
| </span> | ||
| <div |
There was a problem hiding this comment.
Hey, the warning should be here like the package size increase one
| let prevLicense: string | undefined = undefined | ||
|
|
||
| // `data.versions` is an object with version keys | ||
| const versions = Object.values(data.versions) as NpmRegistryVersion[] |
There was a problem hiding this comment.
I think we should only compare to the last version, there is some logic here that could be reused (you can move it into its own utility file in shared too
npmx.dev/app/composables/useInstallSizeDiff.ts
Lines 19 to 39 in 1dd1be9
|
Hi @ghostdevv thanks for the comments. I am working on them. |
…e checked for the last version
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/LicenseChangeWarning.vue (1)
31-35: Use a live region role for the async warning panel.Line 31 renders after async data resolution;
aria-labelon a plaindivis not reliably announced. Add a live-region role (status/alert) so screen-reader users are notified.♿ Suggested tweak
<div v-if="changes && changes.length > 0" class="border border-amber-600/40 bg-amber-500/10 rounded-lg mt-1 gap-x-1 py-2 px-3" + role="status" + aria-live="polite" :aria-label="$t('package.versions.license_change_help')" >
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1167677c-ef72-4f68-b6b4-6b6e3ed2725a
📒 Files selected for processing (5)
app/components/LicenseChangeWarning.vueapp/composables/useLicenseChanges.tsapp/pages/package/[[org]]/[name].vuei18n/locales/en.jsoni18n/locales/tr-TR.json
✅ Files skipped from review due to trivial changes (2)
- i18n/locales/en.json
- i18n/locales/tr-TR.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/composables/useLicenseChanges.ts
app/pages/package/[[org]]/[name].vue
Outdated
| //delete | ||
| watch( | ||
| resolvedVersion, | ||
| newV => { | ||
| console.log('#1 resolvedVersion ', newV) | ||
| }, | ||
| { immediate: true }, | ||
| ) |
There was a problem hiding this comment.
Remove the temporary debug watcher before merge.
Line 466 prints to console on every version update and triggers the existing no-console lint warning.
🧹 Proposed cleanup
-//delete
-watch(
- resolvedVersion,
- newV => {
- console.log('#1 resolvedVersion ', newV)
- },
- { immediate: true },
-)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //delete | |
| watch( | |
| resolvedVersion, | |
| newV => { | |
| console.log('#1 resolvedVersion ', newV) | |
| }, | |
| { immediate: true }, | |
| ) |
🧰 Tools
🪛 GitHub Check: 🤖 Autofix code
[warning] 466-466: eslint(no-console)
Unexpected console statement.
|
@ghostdevv Hi |
| "license_change_item": "from {from} to {to} at version {version}", | ||
| "filter_tooltip": "Filter versions using a {link}. For example, ^3.0.0 shows all 3.x versions.", | ||
| "changed_license": "The license was changed {license_change}", |
There was a problem hiding this comment.
Currently it's nesting the keys right? I think they should be one key, so it'd be like The license was changed {from} to {to}. I also don't think we need the {version} since it's on the version page, wdyt?
There was a problem hiding this comment.
agree on that. thanks, as we changed the logic from checking all versions to last version, there is really no meaning to show it anymore, even if it runs the logic on resolvedVersion, which user can change by selecting from version selector.
I will check the keys, so that it will be simpler.
| /** | ||
| * Composable to detect license changes across all versions of a package | ||
| */ | ||
| export function useLicenseChanges( |
There was a problem hiding this comment.
Sorry, another thing here - it may be worth doing this on the server so we don't have to fetch the packument on the client. I think I linked this before, but it's also had some improvements since then. Let me know if you want some help, or don't understand!
There was a problem hiding this comment.
ok, i am not familiar with nuxt (working with next.js, react and vue, mostly) but i will check this example in detail and run my composable on server. If i have some questions i will return you. Thanks for pointing this out to me.
Hmm maybe they got stuck 🤔 I merged with main and pushed which seems to have fixed CI! |
2 participants
🔗 Linked issue
fixes #1826
🧭 Context
added info/warning beneath the license info, "changed!" if there is a change in the license with additional tooltip which shows info about the change, similar to "from x to y at version a.b".
Also modified the Turkish language support too.
📚 Description
as mentioned in the issue, sometimes license changes and it is important to notify the potential user of the package. This pull request is done to make sure that if there is a change in the license the user will be able to see it and also will get info about the details of the change (like from x license to y license and at version a.b.c)
The following changes are done to the codebase:
modified the [name].vue file: added a new props to LicenseDisplay component.
new composable, useLicenseChanges is written.
In the LicenseDisplay component, changes were made to use this composable and add new element for displaying info about the changed license.
Additionally, en.json and tr-TR.json and schema.json files are modified, to add i18n infrastructure and add Turkish language support (for other languages, additional changes are required)
before:
after:
