Skip to content

feat(premium-analytics): resolve internal packages for build and types#49189

Open
chihsuan wants to merge 7 commits into
trunkfrom
wooa7s-1320-configure-workspace-and-typescript-for-internal-packages
Open

feat(premium-analytics): resolve internal packages for build and types#49189
chihsuan wants to merge 7 commits into
trunkfrom
wooa7s-1320-configure-workspace-and-typescript-for-internal-packages

Conversation

@chihsuan
Copy link
Copy Markdown
Member

@chihsuan chihsuan commented May 27, 2026

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.

  • tsconfig paths alias @jetpack-premium-analytics/* → ./packages/*/src, so tsgo and editors resolve cross-package imports to their TypeScript source.
  • typecheck script (tsgo --noEmit) + @typescript/native-preview devDep, so pnpm --recursive typecheck covers 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/build identity 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 in node_modules under that specifier (upstream @wordpress/build resolves it via require.resolve). Two monorepo rules collide:

  • the repo name lint (lint-project-structure.sh) rejects the bare @jetpack-premium-analytics/* scope, and
  • pnpm rejects a _@…-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 to packages/init — and add a link:packages/<dir> dep on this package's package.json when 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-root pnpm-workspace.yaml) also works — I verified it produces the same externalized module_dependencies — but it was rejected in favor of link: because:

  • It edits a shared, repo-root file for one package's internals. The root pnpm-workspace.yaml globs at projects/*/*; these modules are nested a level deeper, so supporting them means premium-analytics-specific entries in a monorepo-wide config.
  • strictPeerDependencies then applies to each internal package. As a workspace member, init must declare every transitive peer itself (e.g. react/react-dom, pulled in by @wordpress/data/icons), or pnpm install fails — extra deps that the link: target (not a managed importer) doesn't need.
  • It adds a lockfile importer stanza per internal package (churn), vs. a single line for link:.
  • It doesn't avoid the naming constraint — a workspace member still can't be _@… (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:

pnpm install                          # completes cleanly, no lockfile changes
cd projects/packages/premium-analytics
pnpm build                            # succeeds
pnpm typecheck                        # tsgo --noEmit, passes (also: pnpm -r typecheck from root)

Optional — confirm the documented cross-package mechanism end-to-end. packages/init already has the documented @automattic/jetpack-premium-analytics-init name, so the only temporary edits are the link: dep and a real import:

  1. Add to projects/packages/premium-analytics/package.json dependencies: "@jetpack-premium-analytics/init": "link:packages/init".
  2. In routes/dashboard/stage.tsx, import and reference the module so esbuild doesn't tree-shake it away:
    import { init } from "@jetpack-premium-analytics/init";
    void init;
  3. pnpm install && pnpm build && pnpm typecheck — all pass, and build/routes/dashboard/content.min.asset.php lists the dependency:
    'module_dependencies' => array( array( 'id' => '@jetpack-premium-analytics/init', 'import' => 'static' ) )

Revert by undoing the two edits above and running pnpm install (pnpm will tear down the now-unused symlink).

@chihsuan chihsuan requested a review from a team as a code owner May 27, 2026 07:24
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack or WordPress.com Site Helper), and enable the wooa7s-1320-configure-workspace-and-typescript-for-internal-packages branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack wooa7s-1320-configure-workspace-and-typescript-for-internal-packages
bin/jetpack-downloader test jetpack-mu-wpcom-plugin wooa7s-1320-configure-workspace-and-typescript-for-internal-packages

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 27, 2026

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

@github-actions github-actions Bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label May 27, 2026
@chihsuan chihsuan self-assigned this May 27, 2026
@chihsuan chihsuan marked this pull request as draft May 27, 2026 07:34
@jp-launch-control
Copy link
Copy Markdown

jp-launch-control Bot commented May 27, 2026

Code Coverage Summary

This 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. 🤷

Full summary · PHP report · JS report

@chihsuan chihsuan force-pushed the wooa7s-1320-configure-workspace-and-typescript-for-internal-packages branch from b1170de to 00fed2d Compare May 27, 2026 07:45
@chihsuan chihsuan removed the request for review from a team May 27, 2026 07:45
@chihsuan chihsuan marked this pull request as ready for review May 27, 2026 08:10
@chihsuan chihsuan requested a review from Copilot May 27, 2026 08:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json paths mapping for @jetpack-premium-analytics/*./packages/*/src to enable cross-package type resolution.
  • Add a typecheck script (tsgo --noEmit) and @typescript/native-preview dev 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

Comment thread projects/packages/premium-analytics/README.md Outdated
Comment thread projects/packages/premium-analytics/README.md Outdated
chihsuan and others added 2 commits May 27, 2026 16:16
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>
@chihsuan chihsuan requested review from a team and retrofox May 27, 2026 08:17
Copy link
Copy Markdown
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +87 to +96
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):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. And, more pressingly, we'll get script kiddie "security researchers" pointing that out even if we have no such confused tooling.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 name must 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

Copy link
Copy Markdown
Member Author

@chihsuan chihsuan May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. @jetpack-premium-analytics/* (current)
  2. @premium-analytics/* — drops the redundant jetpack-
  3. 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.

"name": "@automattic/jetpack-premium-analytics",

Either way I'll rewrite the README to lead with the "internal-only, symlink-resolved" framing and explicitly explain the dual naming.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated d4da9ca.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this one been reported to the Gutenberg folks?

Copy link
Copy Markdown
Member Author

@chihsuan chihsuan Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
@dognose24
Copy link
Copy Markdown
Contributor

Overall direction LGTM — the type/IDE half is genuinely independent and useful, and the link: rationale over registering in pnpm-workspace.yaml reads cleanly. Two thoughts:

1. Sequencing — +1 to landing #48089 first

Once #48089 lands (the local patch mirroring WordPress/gutenberg#78822 + WordPress/gutenberg#77465 across the monorepo via pnpm-workspace.yaml), the identity model this PR depends on changes: package.json#name becomes the script-module ID and the init rename arrives monorepo-wide. The current @jetpack-premium-analytics/* alias key and the "dual naming is structural" README section both collapse out, and the init rename in this PR overlaps with #48089's. Merging this PR now means a near-immediate follow-up to undo most of those bits — which matches @retrofox's "reduce #49189 to the parts that survive name-based identity" follow-up in #48089.

If there's value in landing something now, the typecheck script + @typescript/native-preview devDep is fully orthogonal to the naming model — that piece could split into its own PR and merge today, with the tsconfig alias + README waiting for #48089.

2. routes/dashboard/package.json naming inconsistency

packages/init was renamed, but routes/dashboard/package.json still has:

"name": "_@jetpack-premium-analytics/dashboard-route"

Routes aren't pnpm workspace members so the _@ invalid prefix doesn't reach pnpm's validation — but the convention is now split: packages/* uses @automattic/jetpack-premium-analytics-*, routes/* still uses _@jetpack-premium-analytics/*. Anyone adding a new route by copy-paste will see the README's "Internal packages" pattern and the routes/dashboard/ example pointing in different directions. Either rename for consistency or add a short note explaining the asymmetry — mild preference for renaming, one fewer footnote.

chihsuan added 2 commits June 3, 2026 10:15
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).
@chihsuan
Copy link
Copy Markdown
Member Author

chihsuan commented Jun 3, 2026

  1. routes/dashboard/package.json naming inconsistency

Thanks for the review! @dognose24 Addressed in 2beeaa9

Comment thread projects/packages/premium-analytics/tsconfig.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docs [Package] Premium Analytics [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants