Draft
Conversation
…s inflection pairity
Contributor
📊 Package size report 0.2%↑
🤖 This report was automatically generated by pkg-size-action |
- Remove ModuleRegistry class and globalThis.requirejs/require references - Require explicit modules via Resolver.withModules() instead of AMD fallback - Rewrite all tests to use module maps instead of AMD loader.define() - Fix imports from 'ember-resolver' to local '@ember/engine/lib/strict-resolver' - Fix string utility imports to '@ember/engine/lib/strict-resolver/string' - Add package.json exports for strict-resolver and string utilities - Add Application integration tests using Resolver.withModules() - Fix typo: rename with-modues-test.js to with-modules-test.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace the classic ember-resolver copy with the StrictResolver from
the polyfill/strict-resolver branch (~120 lines vs ~450)
- Integrate into Engine: add modules/plurals properties, modify
resolverFor() to create StrictResolver when modules is set
- Modules use relative ./paths (normalized by stripping ./ and extensions)
- No pods, no scoped packages, no AMD — simple type:name resolution
- Remove classic-only utilities (cache, class-factory, string helpers)
- Remove classic-only tests (pods, string utilities, custom prefixes)
- Rewrite tests for StrictResolver API using modules = {} pattern
- Application integration tests use Application.create({ modules: {...} })
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…olver - Restore cache.js and string.js with proper inflectors (dasherize, classify, underscore, decamelize) and their caching - Import dasherize from string utilities in StrictResolver instead of inlining - Restore string utility tests (classify, dasherize, decamelize, underscore) with create-test-function helper - Add back tests for all lookup types: component, modifier, template, view, route, controller (not just service/adapter/helper) - Restore full normalization test coverage from original test suite - Add config plural override test - Fix setup-resolver: modules stays in function scope, not module scope - Re-export string utilities from package.json Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Revert package.json to original (no export changes needed)
- Add strict-resolver scenario in smoke-tests/scenarios/scenarios.ts
- Add strict-resolver-test.ts with acceptance + unit tests that exercise
an Application using modules = {} with import.meta.glob
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nt types - Add application template with service injection and hbs component - Add index route (/) with model - Add nested posts route with sub-route posts/show (/:post_id) - Add hbs component (site-header.hbs) to test classic template resolution - Add gjs component (post-card.gjs) to test modern component resolution - Acceptance tests cover: index, application, sub-routes, dynamic segments, hbs components, gjs components, service injection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Destroy Application in test afterEach so namespaces don't leak into
subsequent tests (caused "Should not have any NAMESPACES after tests"
failure in the Enumerable suite).
- Pass required BootOptions to app.visit('/', {}) for type-check.
- Drop the reference to the non-existent ember/no-private-routing-service
eslint rule.
- Rewrite classify/dasherize/decamelize/underscore tests to call qunit's
test() directly so eslint no longer misreads the 2nd arg as an expect
count and const is at module scope.
- Hoist classifyReplace1/2 in strict-resolver/string.js to module scope.
- Mark decamelize/underscore as @Private and register all four string
helpers (and Engine's modules/plurals) in tests/docs/expected.js so
docs coverage passes.
- Regenerate package.json with the three strict-resolver path aliases
so the package preparation diff check stays clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ember/test-helpers' setApplication() does
`const Resolver = application.Resolver; Resolver.create({namespace})`,
which threw TypeError in strict-resolver apps because Resolver was
only declared, never assigned. Provide a default Resolver with a
create() that returns a StrictResolver built from namespace.modules
and namespace.plurals. resolverFor() now just delegates to Resolver,
so classic apps that override Resolver keep working as before.
Also narrow the smoke-test globs to the extensions documented for the
strict resolver (js/ts/gjs/gts for code, hbs for route templates) and
convert site-header to a template-only .gjs — raw .hbs components
without a paired .js file still aren't wrapped by the strict resolver.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review feedback on the strict resolver boiled down to three things:
proper pluralization with customization, nested-colocation component
lookup, and dead code in strict-resolver/string.js.
Pluralization: #plural now handles -s/-ss/-sh/-ch/-x/-z suffixes,
consonant + y, and the common irregulars (child/children,
person/people, man/men, woman/women, mouse/mice, tooth/teeth,
foot/feet). The plurals constructor option still overrides anything,
so a user can opt their "childs" type back in.
Nested colocation: added #nestedColocationLookup so component:my-widget
resolves to ./components/my-widget/index.{js,ts,gjs,gts} — the common
pattern for a class paired with a template in a folder. Direct module
matches still take precedence.
Dead code: classify and underscore weren't used by the resolver or
anything else in ember-source, so they're gone (along with their test
files). decamelize is collapsed inside dasherize's implementation.
The setStrings/getStrings/getString runtime string table was also
unused; removed. dasherize loses its yuidoc @method tag since the same
name is already documented via @ember/-internals/string.
Tests: cover -s/-es suffix rules, consonant-y, irregular plurals,
custom plural overriding an irregular, and the three colocation
cases (component/helper/modifier, plus direct-wins-over-colocation).
tests/docs/expected.js drops the entries whose only source was the
trimmed strict-resolver/string.js.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-tests-update Adapt strict resolver tests, remove AMD dependencies
NullVoxPopuli
changed the title
Adds smoke-tests/scenarios/strict-resolver-substates-test.ts — a
sibling to basic-test.ts that builds a full v2 app wired up with the
strict resolver (modules: { ...import.meta.glob(...) }) and exercises
Ember's auto-generated loading/error substates through real route
transitions. Covers:
- visiting / (plain route + template)
- visiting /slow (pending model, renders
templates/slow-loading.hbs
while the gate service holds
the promise)
- visiting /broken (rejected model, renders
templates/broken-error.hbs)
- visiting /posts and /posts/:id (nested and dynamic sub-routes,
same as the existing scenario
but under this flavor of the
app setup)
To make the loading/error substates actually kick in under the strict
resolver, flip StrictResolver.moduleBasedResolver = true. Router._
buildDSL() reads that flag via _hasModuleBasedResolver() and only
creates the synthetic ${name}_loading / ${name}_error routes when
it's set. Without the flag, Ember never asks the resolver for the
substate templates, so this test couldn't observe them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
modulePrefix was only ever read by Namespace#toString for debug names, not by the strict resolver itself — the resolver keys off relative module paths. Dropping it alongside the unused config import keeps the scenario honest about what the strict resolver actually needs from an app. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g-error-states Scenario test: loading and error substates with strict resolver
NullVoxPopuli
commented
Apr 20, 2026
Contributor
Author
There was a problem hiding this comment.
how much do we care about keeping this cache around?
Contributor
Author
There was a problem hiding this comment.
(it's from ember-string)
NullVoxPopuli
commented
Apr 20, 2026
NullVoxPopuli
commented
Apr 20, 2026
NullVoxPopuli
commented
Apr 20, 2026
NullVoxPopuli
commented
Apr 20, 2026
NullVoxPopuli
commented
Apr 20, 2026
Feedback from the review of PR #21303: - "how much do we care about keeping this cache around?" (it's from ember-string) — drop the Cache helper and its test file. dasherize is now a plain `replace.toLowerCase().replace` in strict-resolver/ string.js. For the workloads the resolver sees (module lookups during boot) the cache is noise. - "we don't need to specify this list -- the class that has `modules = ` assignable on it should be able to specify their inflection rules" — drop the built-in IRREGULAR_PLURALS table and the -s/-es / consonant-y suffix rules. Pluralization is back to naive `type + 's'`, matching ember-resolver's behavior. Consumers that want children / people / buses register them up-front via the `plurals` constructor option, same as they already do for `config`. - "is this true? does ember-resolver do this?" — the regex-based rules weren't in ember-resolver either; they're gone alongside the irregulars. - "let's remove this function, I think" — delete the setupResolver helper and its file; basic-test.js now instantiates StrictResolver directly in beforeEach. - "can we also add a strict application to the v2 app scenarios? I thiiiiiink we just need to overwrite the app.js in that scenario" — yes: add `strictResolver` as a variant of v2AppScenarios (in addition to embroiderVite). basic-test.ts now runs against both v2 configurations. Tests updated to match: the suffix/irregular/y→ies cases are removed; one test left behind proves that registering a custom plural still lets you do `child: 'children'` explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI revealed that basic-test.ts includes a route template (`templates/
example-gjs-route.gjs`) that exports a `.gjs` Component class — a v2
convention that works under embroiderVite (via compat-modules) but
doesn't render with the strict resolver: owner.lookup('template:
example-gjs-route') hands back the Component and rendering fails to
produce [data-test=\"model-field\"], so the acceptance test fails.
That's a real gap worth fixing, but it belongs in a separate PR that
can focus on template-as-component lookup under the strict resolver.
For now, drop the v2AppScenarios variant and leave the strictResolver
function available via strictAppScenarios (used by the dedicated
strict-resolver-test.ts / strict-resolver-substates-test.ts).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to review: with cache.js gone, `string.js` was a 9-line file exporting just dasherize. Inline it as a private helper in strict-resolver.ts and delete the strict-resolver/ subdirectory (plus dasherize_test.js — dasherize's behavior is exercised via resolver.normalize(...) tests in basic-test.js). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-review-cleanup Address review: drop cache, simplify pluralize, collapse setup-resolver
NullVoxPopuli
commented
Apr 20, 2026
The strictResolver variant of v2AppScenarios was added in 9b0f351 so basic-test.ts also runs against a strict app. It started failing on the Acceptance | example gjs route test with: TypeError: Cannot read properties of undefined (reading 'push') at new PageTitle (app-…js:…) at ClassicHelperManager.createHelper … Root cause: v2-app-template's application.gjs contains `{{pageTitle "V2AppTemplate"}}`. The pageTitle helper does `@service('page-title')`, so rendering the application template triggers `owner.lookup('service:page-title')`. Under the classic ember-resolver + compat-modules pipeline that service is surfaced automatically from the addon. With the strict resolver, addons don't contribute modules unless the app explicitly wires them into `modules` — which we weren't doing — so the service lookup missed, the helper constructed with `this.tokens` undefined, and the `.push` call threw. Explicitly register the addon service in the variant's app.js: import PageTitleService from 'ember-page-title/services/page-title'; modules = { './services/page-title': { default: PageTitleService }, … }; This matches the intended strict-resolver ergonomics (no auto addon registration; the app declares its modules) and is also what the RFC asks consumers to do. basic-test.ts now passes against the strictResolver variant, alongside strict-resolver-test.ts and strict-resolver-substates-test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ver-basics Register ember-page-title service in the strict-resolver scenario
Document the edge cases of the resolver's "use `.default` if it
exists, else use the whole value" rule (from
#extractDefaultExport). The existing basic-test covers the
straightforward cases — a `{ default: X }` module yields X, and a
plain shorthand value is returned as-is — but not what happens when
the shorthand value *itself* carries a `default` property, or when
`.default` is falsy. Added three new tests:
- shorthand value whose own `default` is truthy is unwrapped
(documenting the gotcha that a class/object accidentally
exposing a truthy `default` will be replaced by that property)
- class with `default = undefined` or no `default` at all is used
directly (the "else" branch of the rule)
- ES-module-shaped `{ default, named, other }` returns only
`default`; extra named exports are ignored
All three assert today's behavior; no resolver changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lt-edge-cases Add tests for the shorthand/`default` selection rule
Four groups of redundancy made the 34-test file mostly noise: 1. Nine near-identical "can lookup a <type>" tests (adapter, service, helper, component, modifier, template, view, route, controller) all exercised the same `type + 's' -> dir` default-lookup path. Collapsed into one table-driven test that asserts each fullName lookup hits the expected module key. 2. `router:main is looked up as just "router" key` and `store:main is looked up as just "store" key` were the same mainLookup behavior with different type strings. Merged into a single test that asserts both. 3. `store:post is looked up as stores/post` just re-tests the default pluralization path already covered by #1. 4. `shorthand module registration (no default wrapper)` is subsumed by the later `shorthand class with a falsy or missing default falls back to the class itself` test (an object without a `default` property is the "class without default" case). 5. `can lookup a nested-colocation component (index file)` and `nested-colocation also works for helpers and modifiers` were two adjacent tests each asserting the same fallback strategy for a different type. Merged into one test that covers all three types. Net: 34 tests -> 21 tests, -113 lines, identical coverage. All three strict-resolver smoke scenarios (strictResolver-basics, strictResolver-strict-resolver, strictResolver-strict-resolver- substates) still pass locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…resolver-tests Trim redundant tests from strict-resolver basic-test
After the basic-test.js cleanup, three cross-file overlaps remain.
None change coverage — each dropped case is already asserted by
another test.
registry_test.ts:
- `resolves shorthand modules (without default wrapper)` is already
covered by basic-test's three shorthand-edge-case tests, and the
Application integration is already proven by `resolves modules
provided via modules property` just above it.
- `resolves router:main via ./router module` just re-checks the
mainLookup path that basic-test covers via `\`type:main\` resolves
to the unpluralized \`type\` module key`; router:main is
registered by Ember itself, not by the strict resolver.
strict-resolver-test.ts:
- `gjs component resolves from modules` and `sub-route with nested
model` both visit `/posts` and inspect `[data-test=post-card]`.
Folded the h2-exists assertion into the count test and dropped
the standalone gjs test.
strict-resolver-substates-test.ts:
- `visiting /posts renders the list` and `visiting a nested dynamic
sub-route renders the detail template` repeat what
strict-resolver-test.ts already checks; the substates scenario
should focus on loading/error state behaviour.
- Removed the now-unused posts/show scaffolding (router entry,
routes/posts.js, routes/posts/show.js, templates/posts.hbs,
templates/posts/show.hbs) to keep the scenario app minimal.
Net: −85 lines across three files, no loss of coverage. All three
strict-resolver smoke scenarios still pass locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing tests exercise a two-level nested route (`posts.show`
via `/posts/42`) but only assert the child template renders. Three
concrete gaps:
1. The parent template isn't asserted to render alongside the
child — a bug where the parent's `{{outlet}}` silently broke
would pass.
2. There's no `templates/posts/index.hbs`, so we don't prove the
resolver can walk `template:posts.index` ->
`templates/posts/index` (the `name.index -> name/index` nested
folder path).
3. No 3-level nesting coverage, so a regression in the
`type:a.b.c -> a/b/c` normalization wouldn't be caught.
Changes:
- Add `this.route('comments')` under `posts.show` and the matching
`routes/posts/show/comments.js` + `templates/posts/show/comments.hbs`.
Give `posts/show.hbs` a `{{outlet}}` so the grandchild has
somewhere to render.
- Add a `templates/posts/index.hbs`.
- Promote the two sub-route tests to also check that the parent
template is still in the DOM (post-cards + `[data-test="posts"]`).
- Add a new "three-level nested route resolves every level" test
that visits `/posts/42/comments` and asserts all three templates
(posts.hbs, posts/show.hbs, posts/show/comments.hbs) resolved
and rendered.
No resolver changes. All three strict-resolver smoke scenarios still
pass locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…verage Fill the nested-route gaps in the strict-resolver scenario
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
90% of this PR is tests
References