Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chihsuan merged 9 commits into from
Jun 5, 2026
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
00fed2d
feat(premium-analytics): add tsconfig paths and typecheck for interna…
chihsuan May 27, 2026
20e6241
Potential fix for pull request finding
chihsuan May 27, 2026
662c887
Potential fix for pull request finding
chihsuan May 27, 2026
d4da9ca
docs(premium-analytics): clarify internal-package naming and rename init
chihsuan May 28, 2026
528458b
Merge branch 'trunk' into wooa7s-1320-configure-workspace-and-typescr…
chihsuan Jun 2, 2026
feeb212
docs(premium-analytics): revert internal-packages README section
chihsuan Jun 3, 2026
2beeaa9
fix(premium-analytics): align route name with internal-package conven…
chihsuan Jun 3, 2026
952a10c
Merge remote-tracking branch 'origin/trunk' into wooa7s-1320-configur…
chihsuan Jun 5, 2026
d810267
docs(premium-analytics): fix stale README reference in tsconfig comment
chihsuan Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions projects/packages/premium-analytics/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ jetpack build packages/premium-analytics # via Jetpack CLI
### Adding a route

1. Create `routes/<name>/package.json`:

```json
{
"name": "<name>-route",
Expand All @@ -53,6 +54,7 @@ jetpack build packages/premium-analytics # via Jetpack CLI
```

2. Create `routes/<name>/stage.tsx` exporting `stage()`:

```tsx
export const stage = () => <div>My new page</div>;
```
Expand All @@ -74,11 +76,29 @@ fixes the template or the minimum WordPress version is 7.0+.
### Init module (`packages/init/`)

Serves two purposes:

1. Sets the dashboard menu icon via `@wordpress/boot` store
2. Forces `@wordpress/build` to track `@wordpress/boot` as a module
dependency — without an init module that imports boot, the build
skips it

## Internal packages (`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):

@anomiex anomiex May 27, 2026

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

@tbradsha tbradsha May 27, 2026

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

@chihsuan chihsuan May 28, 2026

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

@chihsuan chihsuan May 28, 2026 •
edited
Loading

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.

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.

jetpack/projects/packages/premium-analytics/package.json

Line 2 in 00fed2d

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

@chihsuan chihsuan May 28, 2026

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.

@chihsuan chihsuan Jun 2, 2026 •
edited
Loading

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.

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

@retrofox retrofox Jun 2, 2026

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.

@retrofox retrofox Jun 2, 2026

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.

@chihsuan chihsuan Jun 3, 2026

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.

@retrofox retrofox Jun 4, 2026

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?

Added a comment here; cc @anomiex @tbradsha


```jsonc
"dependencies": { "@jetpack-premium-analytics/<dir>": "link:packages/<dir>" }
```

## File structure

```
Expand Down
4 changes: 4 additions & 0 deletions projects/packages/premium-analytics/changelog/add-internal-package-resolution
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: added

Add a tsconfig paths alias and typecheck script so internal packages/* resolve for types/IDE, and document how to wire cross-package imports for the build.
2 changes: 2 additions & 0 deletions projects/packages/premium-analytics/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"scripts": {
"build": "wp-build && mkdir -p build/modules/boot && cp shims/boot-asset.php build/modules/boot/index.min.asset.php",
"build-production": "NODE_ENV=production wp-build && mkdir -p build/modules/boot && cp shims/boot-asset.php build/modules/boot/index.min.asset.php",
"typecheck": "tsgo --noEmit",
"watch": "wp-build --watch"
},
"wpPlugin": {
Expand Down Expand Up @@ -38,6 +39,7 @@
},
"devDependencies": {
"@babel/core": "7.29.0",
"@typescript/native-preview": "7.0.0-dev.20260225.1",
"@wordpress/build": "0.13.0",
"browserslist": "4.28.2"
}
Expand Down
9 changes: 9 additions & 0 deletions projects/packages/premium-analytics/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
{
"extends": "jetpack-js-tools/tsconfig.base.json",
"compilerOptions": {
// Resolve cross-package imports between internal `packages/*` modules
// (`@jetpack-premium-analytics/<dir>`) to their TypeScript source for
// type-checking + IDE. The build resolves the same specifier separately (see
// README → "Internal packages"); this keeps tsc/esbuild and `tsgo` in sync.
Comment thread
chihsuan marked this conversation as resolved.
Outdated
"paths": {
"@jetpack-premium-analytics/*": [ "./packages/*/src" ]
Comment thread
manzoorwanijk marked this conversation as resolved.
}
},
"include": [ "routes/**/*", "packages/**/*" ]
}
Loading