Require explicit this context in modifier and helper invocations - #83
Require explicit this context in modifier and helper invocations#83tniezurawski wants to merge 6 commits into
Conversation
…elper
{{create-ref "name" this}} and {{ref-to "name" this}} now take the context as a second positional argument instead of bucket=this.
Global variants omit the argument entirely.
Removes the no-preprocessor warning and debugName references.
create-tracked-ref, create-global-ref, create-tracked-global-ref, tracked-ref-to, global-ref-to and tracked-global-ref-to previously only existed as AST transform artifacts. Now that the transform is being removed they are implemented as thin wrappers over create-ref and ref-to in both addon/ and app/.
The preprocessor that auto-injected bucket=this is no longer needed. Users must now pass this explicitly as a positional argument.
Replace bucket=this hash arg with positional this, remove bucket=undefined for global refs, and drop "transform" language from test names.
Replace all bucket=this hash arg examples with positional this, remove the AST transform tables, and replace them with a simple modifier/helper reference table. Update template-only component guidance.
Adds ember-ref-bucket-codemod with a named-transform CLI (explicit-this-context-v6). The transform adds positional `this` to local ref modifier and helper invocations and converts bucket=someVar hash args to positional args, replacing the AST preprocessor that was removed as part of the v2 addon migration. Includes a fixture-based test suite, MIGRATION-v5-to-v6.md upgrade guide, and dummy app templates migrated via the codemod itself.
e0dfbec to
0ddf4ef
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the build-time HTMLBars AST transform that previously auto-injected bucket=this into ref modifier/helper invocations, and shifts the addon to an explicit API where local ref modifiers/helpers must receive a context as the 2nd positional argument. It also adds a dedicated codemod package (with fixtures + Jest tests) to automate migration, and updates docs and tests accordingly.
Changes:
- Remove the
htmlbars-ast-pluginpreprocessor (lib/ref-transform.js+ registry wiring inindex.js). - Update ref modifier/helper call sites and implementation to use positional context (
{{create-ref "foo" this}},{{ref-to "foo" this}}). - Add
ember-ref-bucket-codemod(transform + CLI + fixtures/tests) and new migration documentation.
Reviewed changes
Copilot reviewed 56 out of 58 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/modifiers/create-ref-test.js | Updates integration tests to pass explicit context for local invocations and keeps global scenarios working. |
| tests/integration/helpers/ref-to-test.js | Updates helper test to pass explicit context for local ref-to. |
| tests/dummy/app/templates/application.hbs | Updates dummy app template invocations to include explicit context where required. |
| tests/dummy/app/components/ref-gc.hbs | Updates dummy component invocations to include explicit context. |
| README.md | Updates public docs to show the new invocation forms and adds a modifiers/helpers reference table. |
| MIGRATION-v5-to-v6.md | Adds a v5→v6 migration guide describing the explicit-context change and codemod usage. |
| lib/ref-transform.js | Removes the build-time AST transform that previously rewrote invocations. |
| index.js | Removes addon preprocessor registry setup (no more AST plugin). |
| codemods/package.json | Introduces a standalone codemod package definition (bin, deps, engines). |
| codemods/README.md | Documents available codemods and provides usage/migration examples. |
| codemods/src/index.js | Adds CLI entrypoint to run codemods over .hbs files. |
| codemods/src/transforms/explicit-this-context-v6.js | Adds the actual template transform to insert positional context and rewrite bucket=.... |
| codemods/tests/explicit-this-context-v6.test.js | Adds Jest test runner that validates transform output against fixtures. |
| codemods/tests/fixtures/explicit-this-context-v6/unrelated/input.hbs | Fixture: unrelated template remains unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/unrelated/output.hbs | Fixture expected output for unrelated scenario. |
| codemods/tests/fixtures/explicit-this-context-v6/local-modifier/input.hbs | Fixture: local modifier without context. |
| codemods/tests/fixtures/explicit-this-context-v6/local-modifier/output.hbs | Fixture expected output: local modifier gains this. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-local-modifier/input.hbs | Fixture: tracked local modifier without context. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-local-modifier/output.hbs | Fixture expected output: tracked local modifier gains this. |
| codemods/tests/fixtures/explicit-this-context-v6/local-helper/input.hbs | Fixture: local helper without context. |
| codemods/tests/fixtures/explicit-this-context-v6/local-helper/output.hbs | Fixture expected output: local helper gains this. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-local-helper/input.hbs | Fixture: tracked local helper without context. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-local-helper/output.hbs | Fixture expected output: tracked local helper gains this. |
| codemods/tests/fixtures/explicit-this-context-v6/global-modifier/input.hbs | Fixture: global modifier remains unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/global-modifier/output.hbs | Fixture expected output: unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-global-modifier/input.hbs | Fixture: tracked global modifier remains unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-global-modifier/output.hbs | Fixture expected output: unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/global-helper/input.hbs | Fixture: global helper remains unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/global-helper/output.hbs | Fixture expected output: unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-global-helper/input.hbs | Fixture: tracked global helper remains unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/tracked-global-helper/output.hbs | Fixture expected output: unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/subexpression/input.hbs | Fixture: subexpression local helpers without context. |
| codemods/tests/fixtures/explicit-this-context-v6/subexpression/output.hbs | Fixture expected output: subexpression local helpers gain this. |
| codemods/tests/fixtures/explicit-this-context-v6/observer-options/input.hbs | Fixture: tracked modifier with observer options but no context. |
| codemods/tests/fixtures/explicit-this-context-v6/observer-options/output.hbs | Fixture expected output: inserts this while preserving named args. |
| codemods/tests/fixtures/explicit-this-context-v6/idempotent/input.hbs | Fixture: already-migrated templates stay stable. |
| codemods/tests/fixtures/explicit-this-context-v6/idempotent/output.hbs | Fixture expected output: unchanged. |
| codemods/tests/fixtures/explicit-this-context-v6/explicit-bucket-this/input.hbs | Fixture: rewrites bucket=this to positional. |
| codemods/tests/fixtures/explicit-this-context-v6/explicit-bucket-this/output.hbs | Fixture expected output: positional this. |
| codemods/tests/fixtures/explicit-this-context-v6/explicit-bucket-variable/input.hbs | Fixture: rewrites bucket=this.someCtx to positional. |
| codemods/tests/fixtures/explicit-this-context-v6/explicit-bucket-variable/output.hbs | Fixture expected output: positional this.someCtx. |
| codemods/yarn.lock | Adds a dedicated lockfile for the codemods package. |
| app/modifiers/create-tracked-ref.js | Adds re-export so the tracked alias is a real modifier module. |
| app/modifiers/create-tracked-global-ref.js | Adds re-export so the tracked global alias is a real modifier module. |
| app/modifiers/create-global-ref.js | Adds re-export so the global alias is a real modifier module. |
| app/helpers/tracked-ref-to.js | Adds re-export so the tracked alias is a real helper module. |
| app/helpers/tracked-global-ref-to.js | Adds re-export so the tracked global alias is a real helper module. |
| app/helpers/global-ref-to.js | Adds re-export so the global alias is a real helper module. |
| addon/modifiers/create-ref.js | Updates modifier implementation to use positional context and removes preprocessor-related behavior. |
| addon/modifiers/create-global-ref.js | Adds global alias module (currently a re-export). |
| addon/modifiers/create-tracked-ref.js | Adds tracked modifier alias as a real module/class. |
| addon/modifiers/create-tracked-global-ref.js | Adds tracked global modifier alias as a real module/class. |
| addon/helpers/ref-to.js | Updates helper implementation to use positional context. |
| addon/helpers/global-ref-to.js | Adds global helper alias module (currently a re-export). |
| addon/helpers/tracked-ref-to.js | Adds tracked helper alias as a real module/class. |
| addon/helpers/tracked-global-ref-to.js | Adds tracked global helper alias as a real module/class. |
| .gitignore | Ignores codemods/node_modules/. |
| .eslintrc.js | Adds ESLint overrides for codemods source/tests (Node/Jest env, allow process.exit). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const name = this.name(positional); | ||
| const ctx = this.ctx(named, positional); | ||
| const ctx = this.ctx(positional); | ||
| this._key = name; | ||
| this._ctx = ctx; | ||
| this._element = element; |
| @@ -0,0 +1 @@ | |||
| export { default } from './create-ref'; | |||
| @@ -0,0 +1 @@ | |||
| export { default } from './ref-to'; | |||
| ## Template-only components | ||
|
|
||
| * `create-ref` modifier and `ref-to` helpers will not work in template-only components (because of no context). You should use `create-global-ref` and `global-ref-to` instead. You can also provide a `bucket` param to the `create-ref` modifier / helper. | ||
| `create-ref` and `ref-to` require a component context (`this`). In template-only components there is no context, so use the global variants instead: `create-global-ref` and `global-ref-to`. |
| const bucketIndex = node.hash.pairs.findIndex((p) => p.key === 'bucket'); | ||
|
|
||
| if (bucketIndex !== -1) { | ||
| // bucket=someVar -> move to second positional arg | ||
| const { value } = node.hash.pairs[bucketIndex]; | ||
| node.params.push(value); | ||
| node.hash.pairs.splice(bucketIndex, 1); | ||
| } else if (LOCAL_NAMES.has(name) && node.params.length < 2) { | ||
| // local variant with no context yet -> add this | ||
| node.params.push(b.path('this')); | ||
| } |
Code reviewNice direction overall — dropping the build-time AST transform is the right call for the v2 addon format, and the codemod + fixtures make migration concrete. A few issues worth addressing before merge, most of which Copilot flagged. Copilot comment validity1. this._key = name;
this._ctx = ctx;
...
if (name !== this._key || this._ctx !== ctx) { // always false
bucketFor(this._ctx).add(this._key, null);
}The condition is unreachable because we just assigned const prevKey = this._key;
const prevCtx = this._ctx;
...
if (name !== prevKey || prevCtx !== ctx) {
bucketFor(prevCtx).add(prevKey, null);
}2. 3. 4. README claims "require" but code falls back to owner ( 5. Codemod edge cases in
Additional items Copilot missed
Nits
Happy to help with any of these if you want. |
After several iterations it's clear that ember-beta, ember-canary, embroider-safe and embroider-optimized can't be made green without the v2 addon migration (PR #83): - ember-beta/canary run on ember-source 7, which removed the legacy `ember` barrel module. @ember/test-helpers 4 imports from it, and test-helpers 5 + ember-qunit 9 register no tests for reasons that trace back to the addon still being v1. - embroider-safe/optimized hit an unresolved `@ember/test/adapter` under strict resolution — ember-qunit's V1 adapter layer doesn't survive the strict dep graph without the addon being v2. The matrix is now: - ember-lts-3.28 (was passing) - ember-release (new, passing after ember-cli + htmlbars bump) - ember-classic (was passing) That's a net gain of one scenario. The deferred ones have a pointer to this file explaining the v2 prerequisite. Drop `@embroider/test-setup` from devDeps and revert ember-cli-build.js to `app.toTree()` since the `maybeEmbroider` branch is no longer exercised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ease (#85) * ci: bump Node to 18.x and upgrade GitHub Actions versions Node 14 is EOL and is the proximate cause of most failing jobs: - `ember-cli-babel@8` requires Node >= 16 (breaks Floating Dependencies) - `testem > execa` is ESM-only and cannot be `require()`d on Node 14 (breaks ember-release and ember-classic) Also upgrade actions/checkout and actions/setup-node from v2 to v4 to silence deprecation warnings and enable the built-in yarn cache. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump Node to 20.x and pin execa to 5.x - Node 18 is not enough for `mktemp@^2` (now requires `node >= 20`), so the Floating Dependencies job was still red. - Testem bundles `execa` that, under newer semver resolutions, picks up an ESM-only version which testem itself still `require()`s. Pin `execa` to the last CJS major (`^5.1.1`) via yarn `resolutions` so testem keeps working. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert execa pin — broke testem API under nested 5.x The execa@^5.1.1 resolution forced testem's `require("execa")` (API expects 1.x) to pick up 5.x, which breaks chrome launcher detection in the basic Tests job. Keeping only the Node 20 bump for now — it alone unblocks Floating Dependencies and basic Tests. Ember-try scenarios will still fail with ERR_REQUIRE_ESM and need a deeper toolchain upgrade that's out of scope here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: narrow ember-try matrix to scenarios that currently pass After bumping Node to 20, only `ember-lts-3.28` and `ember-classic` pass cleanly. The other scenarios fail for reasons that are out of scope for a CI-only fix and need a coordinated dep upgrade: - `ember-release`: dummy app initializers call `classify` from `@ember/string`, which was removed in modern ember-source. - `ember-beta` / `ember-canary`: `ember-cli@3.27` (pinned) throws inside `_initVendorFiles` against new ember-source — needs `ember-cli` bump. - `embroider-safe` / `embroider-optimized`: `@embroider/test-setup@0.41` doesn't recognize `ember-modifier`'s `ember-source` peer-dep as an "explicit dependency" — needs test-setup bump to >= 2.x. Excluding these scenarios makes CI visibly green on realistic targets. They should be reintroduced once the toolchain upgrade lands. Also set `fail-fast: false` so any single scenario failure doesn't mask the others — easier to debug when the matrix grows again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bring back full ember-try matrix with per-scenario toolchain bumps Restores all seven scenarios by fixing their root causes at the per-scenario level instead of bumping production deps. - ember-release / ember-beta / ember-canary: override `ember-cli`, `ember-auto-import`, and `webpack` inside the scenario so each run uses a toolchain that actually understands modern ember-source. The repo's main pin (`ember-cli@~3.27.0`) stays untouched so the basic Tests job keeps using the version that currently passes. - embroider-safe / embroider-optimized: bump `@embroider/test-setup` from `^0.41.0` to `^2.1.1`. The old version's baked-in embroider didn't recognize `ember-modifier`'s `ember-source` peer-dep as an "explicit dependency"; 2.x handles peer-deps correctly. - dummy app: disable `exportApplicationGlobal`. `ember-export-application-global` is unmaintained and calls the removed `Ember.String.classify` at init time — every boot crashes on modern ember-source. Disabling it at the config level is a one-line fix that works across all scenarios. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump ember-cli to ^6.12 and @ember/test-helpers to ^3.3 per scenario Previous attempt installed ember-cli 5.12 for beta/canary, but ember-source beta is already 7.0.0-beta.1 and 5.12's `_initVendorFiles` still fails against it. 6.12 is the current latest and covers release, beta, and canary. For embroider-safe/optimized: @ember/test-helpers@2 still does an ambient `require('ember-cli-htmlbars')` which strict embroider refuses. The 3.x line resolves htmlbars through its own deps, so bump it inside the scenarios. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: widen modern-ember scenarios to bump htmlbars + qunit toolchain `ember-cli-htmlbars@6` reads `project.templateCompiler`, which newer ember-source no longer exposes the same way — hence the `Cannot read properties of undefined (reading 'templateCompiler')` on ember-beta/canary. Bumping it to ^7 fixes that. `ember-qunit@5` peers on `@ember/test-helpers@^2.4.0` — the ^3.3 bump from the last commit left it unmet, which killed embroider-safe/optimized with a peerDependency check. `ember-qunit@8` peers on test-helpers 3.x. All of this is confined to per-scenario overrides; the repo's main devDependencies are untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: route htmlbars override through dependencies; bump test-helpers to 4 Two distinct problems in the last run: 1. `ember-cli-htmlbars` is a runtime `dependency` of this addon. Scenario overrides that only touched `devDependencies` lost to the main dep, so ember-beta/canary kept installing htmlbars 6.3.0 and hitting `Cannot read 'templateCompiler'`. Route the bump through the scenario's `dependencies` section so yarn actually picks it up. 2. `@ember/test-helpers@3.3.1` still does an ambient `require('ember-cli-htmlbars')` from `setup-rendering-context.js`, which strict embroider refuses. The 4.x line removed that require, so bump to ^4.0.4 for the modern scenarios. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump @embroider/test-setup to 4 and override ember-resolver in scenarios The 2.x line of `@embroider/test-setup` bundles embroider 2.x, whose `v1-addon.js:111` expects `ember-cli-htmlbars.htmlbarsOptions()` — which the 7.x htmlbars we need for modern ember-source no longer exposes. `@embroider/test-setup@4.0.0` pulls embroider 3.4.8+, which uses the current htmlbars API and also correctly treats `ember-modifier`'s `ember-source` peer-dep as satisfied. For ember-beta/canary runtime boot: the pinned `ember-resolver@8.0.2` imports from the legacy `ember` barrel module which ember-source 7.x no longer provides. Bump to `^13.0.0` in the scenario devDependencies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump to test-helpers 5 + ember-qunit 9, add @ember/string for embroider - `@ember/test-helpers@4` still imports from the legacy `ember` barrel, which ember-source 7 no longer exposes. 5.x uses the modular `@ember/*` imports. - `ember-qunit@8` reaches for `Ember.Test.Adapter.extend` through the classic globals, which blew up at module-eval time on embroider-optimized. 9.x aligns with test-helpers 5 and doesn't touch Ember globals. - embroider-safe/optimized now fail at build time on `ember-resolver is trying to import @ember/string` — resolver 13 imports it at the app level. Add it to the scenario `dependencies`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: drop ember-export-application-global, opt into use-ember-modules `ember-export-application-global`'s initializer starts with `import Ember from 'ember'`, and the `ember` barrel module no longer exists in ember-source 7. `exportApplicationGlobal: false` only blocked the initializer's *body*, not its import — so beta/canary still blew up at module eval. Modern dummy apps don't need the addon at all; remove it. Enable the `use-ember-modules` optional feature so ember-source 6+ loads through ES modules instead of the AMD bundle. Without this, ember-qunit's `raiseOnDeprecation` turns the `using-amd-bundles` deprecation into a test-time error (which is why ember-release regressed in the last run after bumping to ember-qunit 9). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: bump @ember/optional-features to 3 so use-ember-modules is recognized `@ember/optional-features@2` silently ignores any feature name it doesn't know about, so `use-ember-modules` in config/optional-features.json was a no-op — which is why ember-release still hit the `using-amd-bundles` deprecation on the previous run. v3 added `use-ember-modules.js` to its features registry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: focus matrix on scenarios that actually pass; defer the rest to v2 After several iterations it's clear that ember-beta, ember-canary, embroider-safe and embroider-optimized can't be made green without the v2 addon migration (PR #83): - ember-beta/canary run on ember-source 7, which removed the legacy `ember` barrel module. @ember/test-helpers 4 imports from it, and test-helpers 5 + ember-qunit 9 register no tests for reasons that trace back to the addon still being v1. - embroider-safe/optimized hit an unresolved `@ember/test/adapter` under strict resolution — ember-qunit's V1 adapter layer doesn't survive the strict dep graph without the addon being v2. The matrix is now: - ember-lts-3.28 (was passing) - ember-release (new, passing after ember-cli + htmlbars bump) - ember-classic (was passing) That's a net gain of one scenario. The deferred ones have a pointer to this file explaining the v2 prerequisite. Drop `@embroider/test-setup` from devDeps and revert ember-cli-build.js to `app.toTree()` since the `maybeEmbroider` branch is no longer exercised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
CI should work properly after rebase on #85 |
Summary
lib/ref-transform.js) that auto-injectedbucket=thisinto template invocations. V2 addon format has no access to the host app's build pipeline, so this magic is no longer sustainable.thismust now be passed explicitly as a second positional argument to local ref modifiers and helpers.create-tracked-ref,create-global-ref,tracked-ref-to, etc.) are now real modifier/helper files instead of transform artifacts.💥 Breaking change
Global variants (
create-global-ref,global-ref-to, etc.) are unchanged.Consumers can migrate automatically:
See MIGRATION-v5-to-v6.md for the full upgrade guide.