feat(ember)!: Update to v2 addon format - #19229
Conversation
53dd617 to
822ed0a
Compare
nicohrubec
left a comment
There was a problem hiding this comment.
Hey, thanks for contributing. A few notes:
- This PR is way too large. Please split it up into smaller pieces so we can review it properly.
- Is there a way to get this done without introducing breaking changes? If no, then we need to wait with this until our next major.
Hi @nicohrubec, thanks for the feedback! I understand the concern about PR size. Unfortunately, the v1 → v2 addon migration is an atomic change — you can't incrementally migrate between formats. The build pipeline, entry point structure, and package.json layout must change together for the addon to function. That said, the core SDK functionality is unchanged — the same instrumentation, error handling, and performance tracking code. The changes fall into two categories:
The actual instrumentation logic in I'm happy to:
Regarding breaking changes — the v2 format requires explicit Let me know how you'd like to proceed! |
822ed0a to
4874c99
Compare
|
@aklkv thanks a lot for doing this!🙏 FWIW: While waiting for this, I realized I didn't use the performance part of the addon, and was able to just use the That also means that the 'breaking change' mentioned in the PR is a very welcome change to reduce the scope of what is delegated to the 'ember' part of the addon. Most is now just handled by the core apis, which it how it should be👍 (the link to Ember v2 addon format is broken in the pr description, btw) |
NullVoxPopuli
left a comment
There was a problem hiding this comment.
This is a good PR.
I understand the maintainer's perspective of wanting it split up tho.
The only way to do so would be rearranging the monorepo a bit to make a separate test app package.
This would mean the goal of that PR is to just delete the dummy app from the existing v1 addon.
That will still be a large pr, and i don't really see a way to get better than 2 large prs for this needed work.
Maintainers,
You mentioned breaking change timing - i suspect that is because all packages in this repo are released at once?
May i introduce you to https://github.com/release-plan/release-plan ?
It allows each package to be published independently with correct semver inference based on changes in git (and labels assessing impact on a pr) it helps a lot!
4874c99 to
34b116c
Compare
34b116c to
3f38914
Compare
3f38914 to
a52e388
Compare
a52e388 to
c99ff39
Compare
c99ff39 to
e2a465d
Compare
e2a465d to
1f697a9
Compare
1f697a9 to
5884677
Compare
|
I left my review and suggestions for this pull request in aklkv#1. Here's my message to @aklkv on Ember Discord:
|
7cbad07 to
03d2cd6
Compare
|
@mydea will you be able to take this over the finish line? |
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
|
Hey folks 👋 wanted to add a data point here, and also flag that this PR looks close to getting auto-closed by the stale bot again — would be a shame, since it's genuinely the fix we're waiting on. We're in the middle of upgrading from This is exactly why moving No pressure at all given everyone's clearly busy, but wanted to add our +1 that this would unblock real Ember 7 upgrades for apps in a similar mid-migration spot. @mydea @nicohrubec — thanks for all the work on this already, and hope it gets a chance to land soon! 🙏 |
|
@jiayingxu while we wait for this to land may I suggest escape hatch which we use today, you can add this to |
|
Thanks for the suggestion @aklkv! We tested this in our application, and the Vite build passes with Ember 7.1 using |
|
there is a way to patch this with
then in your repo
|
|
Rebased this onto current |
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I created a new PR based on this because there were so many conflicts, but thank you a lot - see #23252 |
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Squashed from PR #19229 (getsentry/sentry-javascript). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebased continuation of #19229 (original author @aklkv) onto current `develop`, with merge conflicts resolved and the failing tests fixed. Migrates `@sentry/ember` from the legacy v1 addon format to the [Ember v2 addon format](https://rfcs.emberjs.com/id/0507-embroider-addons), so the package works with both classic Ember builds and Embroider-optimized builds and no longer depends on `@embroider/macros` at runtime. See #19229 for the full description of the migration. ## Why a new branch The original PR was ~1900 commits behind `develop` and conflicting. Rather than resolve the same conflicts twice across its two commits, I squashed them into one and rebased against the final state once. The subtle part: the migration renamed `packages/ember/addon/` → `src/`. Git therefore saw develop's later behavioral changes to those files as edits to *deleted* files and did **not** surface them as conflicts. Several develop-side changes had to be ported into the new `src/` files by hand: - **Span ops** (#22669, #23086) — route hooks now emit `op: 'function'` with a `code.function.name` attribute; the runloop uses `ui.task`; the transition span uses `router`. `instrumentRoutePerformance.ts` still carried the old `ui.ember.route.*` ops and had to be updated. - **URL attributes** (#22095, #22415) — `url.path` / `url.full` / `url.template` on router spans, reconciled onto the PR's restructured `instrumentEmberAppInstanceForPerformance.ts`. ## Build/tooling reconciliation - Re-added the nested `typescript: ~5.8.0` devDependency pin. `develop` upgraded to TypeScript 7 (the native compiler, which drops `typescript/lib/tsc`), and glint's declaration build needs the classic JS compiler — the same stop-gap `develop` already applies to ember (see #19435). Without the pin the declaration build fails with `ERR_PACKAGE_PATH_NOT_EXPORTED`. - Bumped `@sentry/browser` / `@sentry/core` from the PR's stale `10.53.1` to `10.67.0` and added `@sentry/conventions` (now imported by the ported instrumentation). - Removed the PR's `import/no-unresolved` oxlint rule (doesn't exist in this repo's oxlint 1.75) and wrapped `URL_FULL` in `filterCollectedUrl()` for the `sdk/no-unfiltered-url-attributes` rule, which now applies since the code lives under `src/**`. ## Test fixes The originally-failing tests came down to three things: - **Span-op port** above — fixed the `captures correct spans for navigation` assertions. - **Missing `traceLifecycle: 'static'`** in the two new e2e apps (`ember-strict-resolver`, `ember-vite`). `develop` made span-streaming the default and disables it in the ember test apps (#22588); the new apps predated that, so their performance tests hung waiting for transaction events that never arrived under streaming. - **Stale assertions** in `ember-strict-resolver`'s `sentry-performance.test.ts`, updated from the old `ui.ember.*` op schema to the new `router` / `function` / `ui.task` ops. All four ember e2e apps pass (`ember-classic` 6/6, `ember-embroider` 6/6, `ember-strict-resolver` 10/10, `ember-vite` 5/5), along with the ember unit tests, lint, and build. Supersedes #19229. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebased continuation of #19229 (original author @aklkv) onto current `develop`, with merge conflicts resolved and the failing tests fixed. Migrates `@sentry/ember` from the legacy v1 addon format to the [Ember v2 addon format](https://rfcs.emberjs.com/id/0507-embroider-addons), so the package works with both classic Ember builds and Embroider-optimized builds and no longer depends on `@embroider/macros` at runtime. See #19229 for the full description of the migration. ## Why a new branch The original PR was ~1900 commits behind `develop` and conflicting. Rather than resolve the same conflicts twice across its two commits, I squashed them into one and rebased against the final state once. The subtle part: the migration renamed `packages/ember/addon/` → `src/`. Git therefore saw develop's later behavioral changes to those files as edits to *deleted* files and did **not** surface them as conflicts. Several develop-side changes had to be ported into the new `src/` files by hand: - **Span ops** (#22669, #23086) — route hooks now emit `op: 'function'` with a `code.function.name` attribute; the runloop uses `ui.task`; the transition span uses `router`. `instrumentRoutePerformance.ts` still carried the old `ui.ember.route.*` ops and had to be updated. - **URL attributes** (#22095, #22415) — `url.path` / `url.full` / `url.template` on router spans, reconciled onto the PR's restructured `instrumentEmberAppInstanceForPerformance.ts`. ## Build/tooling reconciliation - Re-added the nested `typescript: ~5.8.0` devDependency pin. `develop` upgraded to TypeScript 7 (the native compiler, which drops `typescript/lib/tsc`), and glint's declaration build needs the classic JS compiler — the same stop-gap `develop` already applies to ember (see #19435). Without the pin the declaration build fails with `ERR_PACKAGE_PATH_NOT_EXPORTED`. - Bumped `@sentry/browser` / `@sentry/core` from the PR's stale `10.53.1` to `10.67.0` and added `@sentry/conventions` (now imported by the ported instrumentation). - Removed the PR's `import/no-unresolved` oxlint rule (doesn't exist in this repo's oxlint 1.75) and wrapped `URL_FULL` in `filterCollectedUrl()` for the `sdk/no-unfiltered-url-attributes` rule, which now applies since the code lives under `src/**`. ## Test fixes The originally-failing tests came down to three things: - **Span-op port** above — fixed the `captures correct spans for navigation` assertions. - **Missing `traceLifecycle: 'static'`** in the two new e2e apps (`ember-strict-resolver`, `ember-vite`). `develop` made span-streaming the default and disables it in the ember test apps (#22588); the new apps predated that, so their performance tests hung waiting for transaction events that never arrived under streaming. - **Stale assertions** in `ember-strict-resolver`'s `sentry-performance.test.ts`, updated from the old `ui.ember.*` op schema to the new `router` / `function` / `ui.task` ops. All four ember e2e apps pass (`ember-classic` 6/6, `ember-embroider` 6/6, `ember-strict-resolver` 10/10, `ember-vite` 5/5), along with the ember unit tests, lint, and build. Supersedes #19229. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Summary
Migrates
@sentry/emberfrom the legacy v1 addon format to the Ember v2 addon format. This modernizes the package to work with both classic Ember builds and Embroider-optimized builds, and removes the runtime dependency on@embroider/macros.Changes
ember-clibuild pipeline withrollup(transpilation →dist/) +tsc(declarations →declarations/)addon-main.cjsusing@embroider/addon-shimas the addon entry pointember-addon.version: 2, updatedexportsmap, addedember-addon.app-jsmapping@embroider/macrosgetOwnConfig()with explicitSentry.init()calls and a newsetupPerformance()exportAPI changes:
init()is now called directly inapp.tsor an initializer (no moreENV['@sentry/ember']config)setupPerformance()must be called from an instance-initializer to opt into performance instrumentation<script>tags inindex.htmlUPGRADE.mdwith detailed migration guideTest & CI updates:
ember-classic,ember-embroider) to use v2 patterns:ENV['@sentry/ember']toSentry.init()inapp.ts<script>tags toindex.html.github/workflows/build.ymlCACHED_BUILD_PATHSfrompackages/ember/*.d.tstopackages/ember/dist+packages/ember/declarationsTypeScript fixes:
"types": ["ember-source/types"]totsconfig.publish.jsonfor Ember module resolution during declaration generation@ember/routing/-private/transition)instrumentFunctiongeneric to avoid tuple mismatch errorsBreaking Changes
ENV['@sentry/ember']config inconfig/environment.jsis no longer supported — useSentry.init()directlysetupPerformance()from an instance-initializer<script>tags inapp/index.htmlCloses JS-217