Address review: drop cache, simplify pluralize, collapse setup-resolver#21326
Merged
NullVoxPopuli merged 4 commits intoemberjs:nvp/strict-resolver-rfc-1132from Apr 20, 2026
Conversation
Feedback from the review of PR emberjs#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>
Contributor
Author
|
Update: the v2AppScenarios `strictResolver` variant exposed a real bug but in a place I don't want to tackle inside this PR. `basic-test.ts` includes `templates/example-gjs-route.gjs` — a route template that is a `.gjs` Component class. Under `embroiderVite` (compat-modules) that renders fine; under the strict resolver `owner.lookup('template:example-gjs-route')` returns the Component class as expected, but rendering never produces `[data-test="model-field"]`, so the acceptance test fails. That's a template-as-component lookup gap in the strict resolver (probably an interaction with how `@model` flows when the "template" is actually a Component). I reverted the v2AppScenarios variant for now; `strictResolver` is still available via `strictAppScenarios` so the dedicated strict-resolver test files keep their coverage. Happy to take the template-as-component fix on as a follow-up PR once this one lands. |
Contributor
There was a problem hiding this comment.
should this now just go in the strict-resolver file?
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>
NullVoxPopuli
approved these changes
Apr 20, 2026
NullVoxPopuli
merged commit dfe4b5c
into
emberjs:nvp/strict-resolver-rfc-1132
39 checks passed
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
2 participants
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.
Addresses the review feedback on #21303:
cache.js— "how much do we care about keeping this cache around? (it's from ember-string)" → dropped.dasherizeis now a plain.replace(...).toLowerCase().replace(...)instrict-resolver/string.js; for module-lookup workloads at boot time the cache was just noise.strict-resolver.ts:134(IRREGULAR_PLURALS) — "we don't need to specify this list -- the class that has `modules = ` assignable on it should be able to specify their inflection rules" → removed the built-in irregulars.strict-resolver.ts:145(consonant-y regex) — "is this true? does ember-resolver do this?" → no, it doesn't. Removed alongside the-s/-essuffix rule.#pluralis back to naivetype + 's', matching ember-resolver. Consumers that wantchildren/people/busesregister them via thepluralsconstructor option (same mechanism already used forconfig).-setup-resolver.js— "let's remove this function, I think" → deleted;basic-test.jsinstantiatesStrictResolverdirectly inbeforeEach.scenarios.ts— "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. AddedstrictResolveras a second variant ofv2AppScenarios(alongsideembroiderVite); it merges a strict-resolverapp.jsover the template's default, so every test that runs againstv2AppScenariosnow also runs against the strict-resolver configuration.package.json:232(generated alias list) — "let's save that for another PR, yea?" → deferred.Tests updated: the suffix/irregular/y→ies cases are gone; one test remains proving that registering
{ child: 'children' }viapluralslets you dochild:aliceexplicitly.🤖 Generated with Claude Code