Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/__tests__/fixtures/size-report-npm-pack.json">
<violation number="1" location="scripts/__tests__/fixtures/size-report-npm-pack.json:5">
P3: This fixture's `entryCount` is still 20, but the `files` array now has 21 entries after adding `THIRD_PARTY_NOTICES.txt`. Update it to 21 so the fixture stays self-consistent.</violation>
</file>
<file name="scripts/size-report-package.mjs">
<violation number="1" location="scripts/size-report-package.mjs:68">
P2: `assertBundledLicenseNotices` is exported here but not declared in `scripts/size-report-package.d.mts`. `check-package.ts` imports it, so TypeScript will not see the export. Add the declaration to the `.d.mts` file.</violation>
</file>
<file name="tsdown.config.ts">
<violation number="1" location="tsdown.config.ts:203">
P2: The notice directory `./dist/src` is hardcoded here while `outDir: 'dist/src'` is set separately above. Define one constant (for example `const outDir = 'dist/src'`) and use it for both `outDir` and the notices path, so the notices cannot drift from the bundle output.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| }; | ||
| } | ||
|
|
||
| export function assertBundledLicenseNotices(entries) { |
There was a problem hiding this comment.
P2: assertBundledLicenseNotices is exported here but not declared in scripts/size-report-package.d.mts. check-package.ts imports it, so TypeScript will not see the export. Add the declaration to the .d.mts file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/size-report-package.mjs, line 68:
<comment>`assertBundledLicenseNotices` is exported here but not declared in `scripts/size-report-package.d.mts`. `check-package.ts` imports it, so TypeScript will not see the export. Add the declaration to the `.d.mts` file.</comment>
<file context>
@@ -65,7 +65,16 @@ export function collectNpmPack(root) {
};
}
+export function assertBundledLicenseNotices(entries) {
+ if (
+ !entries.some((entry) => entry.path === 'dist/src/THIRD_PARTY_NOTICES.txt' && entry.size > 0)
</file context>
| dropAmbientDeclarationImports, | ||
| bundledLicenseNotices( | ||
| import.meta.dirname, | ||
| fileURLToPath(new URL('./dist/src', import.meta.url)), |
There was a problem hiding this comment.
P2: The notice directory ./dist/src is hardcoded here while outDir: 'dist/src' is set separately above. Define one constant (for example const outDir = 'dist/src') and use it for both outDir and the notices path, so the notices cannot drift from the bundle output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tsdown.config.ts, line 203:
<comment>The notice directory `./dist/src` is hardcoded here while `outDir: 'dist/src'` is set separately above. Define one constant (for example `const outDir = 'dist/src'`) and use it for both `outDir` and the notices path, so the notices cannot drift from the bundle output.</comment>
<file context>
@@ -195,7 +196,13 @@ export default defineConfig({
+ dropAmbientDeclarationImports,
+ bundledLicenseNotices(
+ import.meta.dirname,
+ fileURLToPath(new URL('./dist/src', import.meta.url)),
+ ),
+ ],
</file context>
| "unpackedSize": 2319, | ||
| "files": [ | ||
| { | ||
| "path": "dist/src/THIRD_PARTY_NOTICES.txt", |
There was a problem hiding this comment.
P3: This fixture's entryCount is still 20, but the files array now has 21 entries after adding THIRD_PARTY_NOTICES.txt. Update it to 21 so the fixture stays self-consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/__tests__/fixtures/size-report-npm-pack.json, line 5:
<comment>This fixture's `entryCount` is still 20, but the `files` array now has 21 entries after adding `THIRD_PARTY_NOTICES.txt`. Update it to 21 so the fixture stays self-consistent.</comment>
<file context>
@@ -1,6 +1,10 @@
"unpackedSize": 2319,
"files": [
+ {
+ "path": "dist/src/THIRD_PARTY_NOTICES.txt",
+ "size": 100
+ },
</file context>
|
I reviewed 7410a5b and found no problems that must change in the code. Not blocking, and fine to take or leave: sort the dependencies by Three open threads from cubic-dev-ai still apply: the missing Checks are still running. Smoke Tests is pending, and it runs I did not build or run anything. I did not observe the plugin's ordering, its cwd exclusion, or its use of Before merge, Smoke Tests must pass. Then add the d.mts declaration, use one output-directory constant in The newer head c803382 deduplicates the license texts in the notice plugin and its test, which covers the last optional note above. It does not touch |
|
The code at c803382 looks good apart from the two open review threads below, which still need a fix or an answer. All 35 checks pass, including check:tooling, which runs the build and check:package, the route this diff changes. There are no conflicts. Not blocking, and you can take or leave these: sort a copy of The Cubic threads on the missing d.mts declaration (#4070 (comment)) and the duplicated dist/src path (#4070 (comment)) still apply, and one lower-priority thread on entryCount also still applies. Please fix or answer them before merge. I did not run the build or the tests myself. |
Summary
Published bundles include third-party code but omitted its license texts. Generate
dist/src/THIRD_PARTY_NOTICES.txtfrom modules retained in each build, including upstream NOTICE files and jpeg-js's source-level notices. Externalized or tree-shaken dependencies drop out automatically. Identical texts are shared, while distinct copyrights remain separate.Retain the Apache-2.0 license and provenance for vendored Maestro fixtures and the Gradle wrapper. Require a non-empty generated notice file in the actual npm tarball.
13 files changed. Packaging and development compliance only; no CLI, Node.js, or device behavior changes.
Validation
Current head:
c80338229d.Missing-notice regression, same test command with the new test added to the base:
0f7e6680077410a5be4bFocused license-generation and package-content tests: 8 passed, including shared-text deduplication, distinct copyrights, minification, external/tree-shaken dependencies, missing license text, and jpeg-js source notices.
pnpm buildgenerated the license report for the real bundle.Full checks passed at
7410a5be4b.pnpm check:affected --base 7410a5be4b --run --head c80338229dalso passed all runnable checks withexactCommit: true.Size versus
7410a5be4b: notices 92,631 → 47,966 bytes; packed tarball 1,543,992 → 1,541,891 bytes. All 27 packages and 57 distinct copyright lines retained.Independent review of the initial change and the compact formatter: no actionable findings.
GitHub CI pending; no device runs required for this packaging-only change.