feat(premium-analytics): resolve internal packages for build and types#49189
feat(premium-analytics): resolve internal packages for build and types#49189chihsuan wants to merge 7 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
b1170de to
00fed2d
Compare
There was a problem hiding this comment.
Pull request overview
Wires up TypeScript/editor resolution for app-internal packages/* modules within @automattic/jetpack-premium-analytics as groundwork for the analytics SPA migration, by adding a tsconfig alias and a package-level typecheck script, plus documentation of the future build-time wiring approach.
Changes:
- Add
tsconfig.jsonpathsmapping for@jetpack-premium-analytics/*→./packages/*/srcto enable cross-package type resolution. - Add a
typecheckscript (tsgo --noEmit) and@typescript/native-previewdev dependency for recursive typechecking. - Document the intended build-time
link:approach for internal packages in the package README.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/premium-analytics/tsconfig.json | Adds TS paths alias for internal packages/* module resolution. |
| projects/packages/premium-analytics/README.md | Documents internal package resolution approach and updates route-creation snippets. |
| projects/packages/premium-analytics/package.json | Adds typecheck script and @typescript/native-preview dev dependency. |
| projects/packages/premium-analytics/changelog/add-internal-package-resolution | Changelog entry for the added type/IDE wiring and docs. |
| pnpm-lock.yaml | Locks the added @typescript/native-preview dev dependency for this importer. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
anomiex
left a comment
There was a problem hiding this comment.
Overall I feel very meh about this wp-build feature. If the packages really are shareable, it may be better to make them actual monorepo packages in projects/js-packages/ instead. But I guess this is wp-build's way of doing bundle splitting, rather that really being about packages.
| App-internal modules discovered by `@wordpress/build`. Types/IDE resolve | ||
| `@jetpack-premium-analytics/<dir>` imports via the `tsconfig.json` `paths` alias | ||
| (`pnpm typecheck`). | ||
|
|
||
| To import one from a route/another package, the build also needs it symlinked in | ||
| `node_modules` under that specifier. Name the package | ||
| `@automattic/jetpack-premium-analytics-<dir>` (a bare `@jetpack-premium-analytics/*` | ||
| name fails the repo name lint and `_@…` is invalid to pnpm), then add a `link:` | ||
| dep in this package's `projects/packages/premium-analytics/package.json` | ||
| (not the repo root `package.json`; routes aren't workspace members): |
There was a problem hiding this comment.
The advice here seems confused. The first paragraph says to name like @jetpack-premium-analytics/<dir>, while the second says that won't work and to do like @automattic/jetpack-premium-analytics-<dir>.
The latter seems more correct to me, as Automattic owns the @automattic/ prefix in npm. In theory someone else could claim @jetpack-premium-analytics/ and put packages there, which confused tooling could theoretically try to use.1
The code sample below and the tsconfig file in this PR do the former, however.
Footnotes
-
And, more pressingly, we'll get script kiddie "security researchers" pointing that out even if we have no such confused tooling. ↩
There was a problem hiding this comment.
I haven't reviewed this and may not get around to it, but it seems premium-analytics will be something used quite globally. Do we even want to prefix it with jetpack-?
There was a problem hiding this comment.
If the packages really are shareable, it may be better to make them actual monorepo packages in projects/js-packages/ instead.
Thanks for the review! @anomiex
To clarify intent: these packages/* aren't shareable. It scoped to premium-analytics only, no plan to publish or reuse across the monorepo. The wp-build packages/* mechanism is just how this app does bundle splitting, not a package boundary. . I'll rewrite the README to lead with that.
The advice here seems confused. The first paragraph says to name like
@jetpack-premium-analytics/<dir>, while the second says that won't work and to do like@automattic/jetpack-premium-analytics-<dir>.
Fair, that section is confusing. The two names aren't a contradiction but the README doesn't say why:
- Package
namemust be@automattic/jetpack-premium-analytics-<dir>(pnpm rejects_@…, repo lint rejects bare@jetpack-premium-analytics/*). - The symlink + import specifier come from the dep key, not the package's
name— so"@jetpack-premium-analytics/<dir>": "link:packages/<dir>"symlinks at that key regardless.
The @jetpack-premium-analytics/* scope came from the existing code and original issue (WOOA7S-1320), but your point is fair. Combined with @tbradsha's question below, I'll collapse everything to @automattic/premium-analytics-<dir> (one identifier for name, dep key, symlink, specifier, tsconfig path).
cc @retrofox
There was a problem hiding this comment.
Just looked more into wp-build and found it doesn't structurally allow it. The internal-package specifier is built as @${packageNamespace}/${dir} , and packageNamespace doubles as the externalization pattern ^@<ns>/ — so setting it to automattic would catch every @automattic/* import as a wp script module and break the build the moment any real @automattic/... dep is added.
So the specifier scope has to be a single-segment, non-conflicting string, and the dual naming (package name vs. specifier) is structural — not something the README can collapse, only explain.
Real options for the specifier scope:
@jetpack-premium-analytics/*(current)@premium-analytics/*— drops the redundantjetpack-- Something explicitly internal-looking like
@jpa-internal/*
I'm leaning option 1 so it match the existing package namespace unless we want to change it.
Either way I'll rewrite the README to lead with the "internal-only, symlink-resolved" framing and explicitly explain the dual naming.
There was a problem hiding this comment.
Has this one been reported to the Gutenberg folks?
There was a problem hiding this comment.
Yep, @retrofox just opened WordPress/gutenberg#78822, which targets exactly this: wpPlugin.packageNamespace carrying three roles and forcing the dual naming. The proposal uses package.json#name as the script-module ID source of truth, so the specifier keeps the real @automattic/… identity end-to-end.
Related constraints already raised upstream: WordPress/gutenberg#77225 (package discovery locked to ./packages/*) and WordPress/gutenberg#75196 (classic scripts depending on script modules — the root of the boot-asset shim documented above).
There was a problem hiding this comment.
Thanks, Chi. We're researching the alternatives we have for addressing these issues upstream. I think it's the proper solution. Otherwise, we'll end up implementing kind of hacked-together ones.
There was a problem hiding this comment.
Update on the upstream angle @tbradsha asked about, since it lands right on the security concern @anomiex raised here.
I've been working on that upstream fix, and it's validated end-to-end downstream in #48089 (real premium-analytics build):
WordPress/gutenberg#78822 makes the script-module ID come from package.json#name instead of @<packageNamespace>/<dir>.
The import specifier then is the real npm name (@automattic/jetpack-premium-analytics-<dir>).
A scope Automattic owns and can register, so the unregistered @jetpack-premium-analytics/* specifier that triggers the bogus dependency-confusion / HackerOne reports simply goes away.
One identifier end-to-end: npm name === import specifier === script-module ID === tsconfig path.
WordPress/gutenberg#77465 (companion) scopes the generated wp_deregister_script_module() to @wordpress/*, so shared non-core modules get Core's deterministic first-wins instead of last-plugin-wins.
On the "dual naming is structural" point (3315652267), that's exactly right under stock wp-build: packageNamespace doubles as the externalization pattern ^@<ns>/, so you can't set it to automattic without catching every real @automattic/* import.
#78822 unbundles those roles; internal packages are externalized by exact name (a precise match set that runs before the namespace wildcard), not by ^@<ns>/, so the real @automattic/... name can be the specifier without ever swallowing genuine @automattic/* deps.
That's what stops the dual naming from being structural.
What it means for this PR:
Once #78822 lands 🤞, the @jetpack-premium-analytics/* tsconfig alias and the "dual naming is structural" README section collapse to a single @automattic/jetpack-premium-analytics-* identifier (the init rename you already did is the same one #48089 needs).
The typecheck script + devDep are orthogonal and stay. Since #48089 changes the identity model that the alias/README depends on, the cleanest approach is to land #48089 first and rebase this down to (typecheck + name-based alias + simplified README). Happy to coordinate the order with you.
There was a problem hiding this comment.
Thanks Damián, Totally agree WordPress/gutenberg#78822/#48089 is the right long-term fix, and I'll rebase onto it once it lands.
But to make it clear, none of my PRs actually wire a cross-package build import. The @jetpack-premium-analytics/* specifier only appears in README examples + one tsconfig alias. If it (plus upstream) would take a while, could we review/merge the type-side to so we can continue working on next? I've reverted the README "dual naming" section, so it's now just the orthogonal bits: typecheck + devDep + the init rename.
Address PR review feedback on the "Internal packages" section: - Lead with scope intent: internal-only, never published, in-tree symlink-only resolution (answers the npm-squatting concern) - Explicitly explain the structural dual naming between the package name field and the wp-build-derived import specifier - Rename packages/init from `_@jetpack-premium-analytics/init` to `@automattic/jetpack-premium-analytics-init` so the codebase matches the documented pattern (the old placeholder is invalid to pnpm)
…ipt-for-internal-packages
|
Overall direction LGTM — the type/IDE half is genuinely independent and useful, and the 1. Sequencing — +1 to landing #48089 first Once #48089 lands (the local patch mirroring WordPress/gutenberg#78822 + WordPress/gutenberg#77465 across the monorepo via If there's value in landing something now, the 2.
"name": "_@jetpack-premium-analytics/dashboard-route"Routes aren't pnpm workspace members so the |
Defer the internal-packages naming docs until the upstream wp-build identity change (gutenberg#78822 / #48089) lands; restore README to trunk.
…tion Rename routes/dashboard to @automattic/jetpack-premium-analytics-dashboard-route to match the packages/* naming (per review), and drop the stale changelog line about README build docs that were reverted. Build output is unchanged (routes key off the directory name).
Thanks for the review! @dognose24 Addressed in 2beeaa9 |
Proposed changes
Foundation work for the analytics SPA migration (WOOA7S-1320): wire up resolution of internal
packages/*modules in@automattic/jetpack-premium-analytics. Scoped to the type/IDE side now (the half that's unambiguous and immediately useful); the build side is documented and verified but intentionally not wired, since no cross-package source import exists yet.pathsalias@jetpack-premium-analytics/* → ./packages/*/src, sotsgoand editors resolve cross-package imports to their TypeScript source.typecheckscript (tsgo --noEmit) +@typescript/native-previewdevDep, sopnpm --recursive typecheckcovers the package.The README note that previously documented the build-side wiring has been reverted: the "dual naming is structural" framing is premised on the stock
@wordpress/buildidentity model, which the upstream change in WordPress/gutenberg#78822 (validated downstream in #48089) collapses to a single@automattic/jetpack-premium-analytics-*identifier. Documenting it now would just be churn, so the docs are deferred until that lands.Why the build side isn't wired here
A route/package importing
@jetpack-premium-analytics/<dir>needs the package symlinked innode_modulesunder that specifier (upstream@wordpress/buildresolves it viarequire.resolve). Two monorepo rules collide:lint-project-structure.sh) rejects the bare@jetpack-premium-analytics/*scope, and_@…-prefixed name as invalid — so the lint's_-prefix escape hatch can't be a pnpm link target.The verified working combination: name the package
@automattic/jetpack-premium-analytics-<dir>— now applied topackages/init— and add alink:packages/<dir>dep on this package'spackage.jsonwhen the first cross-package source import lands. This keeps the committed diff minimal and avoids fabricated wiring.Why not register the packages in
pnpm-workspace.yaml?Making the internal packages pnpm workspace members (adding a nested glob like
projects/packages/premium-analytics/packages/*to the repo-rootpnpm-workspace.yaml) also works — I verified it produces the same externalizedmodule_dependencies— but it was rejected in favor oflink:because:pnpm-workspace.yamlglobs atprojects/*/*; these modules are nested a level deeper, so supporting them means premium-analytics-specific entries in a monorepo-wide config.strictPeerDependenciesthen applies to each internal package. As a workspace member,initmust declare every transitive peer itself (e.g.react/react-dom, pulled in by@wordpress/data/icons), orpnpm installfails — extra deps that thelink:target (not a managed importer) doesn't need.link:._@…(invalid to pnpm) and still can't be the bare@jetpack-premium-analytics/*scope (fails the lint), so it would need the@automattic/…name anyway.link:gives identical build resolution while keeping everything self-contained to the package and the root config untouched.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Requires Node 24 (repo
engineStrict— e.g.nvm use 24).Baseline — the type/IDE side that ships in this PR:
Optional — confirm the documented cross-package mechanism end-to-end.
packages/initalready has the documented@automattic/jetpack-premium-analytics-initname, so the only temporary edits are thelink:dep and a real import:projects/packages/premium-analytics/package.jsondependencies:"@jetpack-premium-analytics/init": "link:packages/init".routes/dashboard/stage.tsx, import and reference the module so esbuild doesn't tree-shake it away:pnpm install && pnpm build && pnpm typecheck— all pass, andbuild/routes/dashboard/content.min.asset.phplists the dependency:Revert by undoing the two edits above and running
pnpm install(pnpm will tear down the now-unused symlink).