Skip to content

Commit bbd7bd8

Browse files
NullVoxPopuliclaude
andcommitted
Address review: drop cache, simplify pluralize, collapse setup-resolver
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) <[email protected]>
1 parent c557ce6 commit bbd7bd8

6 files changed

Lines changed: 44 additions & 133 deletions

File tree

packages/@ember/engine/lib/strict-resolver.ts

Lines changed: 1 addition & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export class StrictResolver implements Resolver {
3636
}
3737

3838
#plural(s: string) {
39-
return this.#plurals.get(s) ?? pluralize(s);
39+
return this.#plurals.get(s) ?? s + 's';
4040
}
4141

4242
resolve(fullName: string): Factory<object> | object | undefined {
@@ -128,36 +128,6 @@ export class StrictResolver implements Resolver {
128128
}
129129
}
130130

131-
// Handle the common irregular English plurals plus the standard -s / -es
132-
// suffix rules. Users can override any type via the `plurals` constructor
133-
// option (including overriding these defaults).
134-
const IRREGULAR_PLURALS: Record<string, string> = Object.freeze({
135-
child: 'children',
136-
man: 'men',
137-
woman: 'women',
138-
person: 'people',
139-
mouse: 'mice',
140-
tooth: 'teeth',
141-
foot: 'feet',
142-
});
143-
144-
const NEEDS_ES_SUFFIX = /(s|ss|sh|ch|x|z)$/;
145-
const ENDS_IN_CONSONANT_Y = /([^aeiou])y$/;
146-
147-
function pluralize(singular: string): string {
148-
let irregular = IRREGULAR_PLURALS[singular];
149-
if (irregular) {
150-
return irregular;
151-
}
152-
if (ENDS_IN_CONSONANT_Y.test(singular)) {
153-
return singular.replace(ENDS_IN_CONSONANT_Y, '$1ies');
154-
}
155-
if (NEEDS_ES_SUFFIX.test(singular)) {
156-
return singular + 'es';
157-
}
158-
return singular + 's';
159-
}
160-
161131
const fileExtension = /\.\w{1,4}$/;
162132
const leadingDotSlash = /^\.\//;
163133

packages/@ember/engine/lib/strict-resolver/cache.js

Lines changed: 0 additions & 35 deletions
This file was deleted.
Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,9 @@
1-
import Cache from './cache';
2-
31
const STRING_DASHERIZE_REGEXP = /[ _]/g;
42
const STRING_DECAMELIZE_REGEXP = /([a-z\d])([A-Z])/g;
53

6-
const DECAMELIZE_CACHE = new Cache(1000, (str) =>
7-
str.replace(STRING_DECAMELIZE_REGEXP, '$1_$2').toLowerCase()
8-
);
9-
10-
const STRING_DASHERIZE_CACHE = new Cache(1000, (key) =>
11-
DECAMELIZE_CACHE.get(key).replace(STRING_DASHERIZE_REGEXP, '-')
12-
);
13-
144
export function dasherize(str) {
15-
return STRING_DASHERIZE_CACHE.get(str);
5+
return str
6+
.replace(STRING_DECAMELIZE_REGEXP, '$1_$2')
7+
.toLowerCase()
8+
.replace(STRING_DASHERIZE_REGEXP, '-');
169
}

packages/@ember/engine/tests/resolver/-setup-resolver.js

Lines changed: 0 additions & 9 deletions
This file was deleted.

packages/@ember/engine/tests/resolver/basic-test.js

Lines changed: 10 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import { module, test } from 'qunit';
22
import { StrictResolver } from '@ember/engine/lib/strict-resolver';
3-
import { setupResolver } from './-setup-resolver';
43

54
module('strict-resolver | basic', function (hooks) {
65
let resolver;
76
let modules;
87

98
hooks.beforeEach(function () {
10-
({ resolver, modules } = setupResolver());
9+
modules = {};
10+
resolver = new StrictResolver(modules);
1111
});
1212

1313
test('can lookup something', function (assert) {
@@ -356,49 +356,14 @@ module('strict-resolver | basic', function (hooks) {
356356
assert.strictEqual(result, 'whatever', 'super-duper-config/environment is found');
357357
});
358358

359-
test('default plural handles -s / -ss / -sh / -ch / -x / -z suffixes', function (assert) {
360-
let cases = {
361-
'./buses/red': 'bus:red',
362-
'./brushes/broom': 'brush:broom',
363-
'./benches/park': 'bench:park',
364-
'./boxes/cardboard': 'box:cardboard',
365-
'./buzzes/loud': 'buzz:loud',
366-
'./classes/math': 'class:math',
367-
};
368-
369-
for (let [modulePath, lookup] of Object.entries(cases)) {
370-
let r = new StrictResolver({ [modulePath]: modulePath });
371-
assert.strictEqual(r.resolve(lookup), modulePath, `${lookup} -> ${modulePath}`);
372-
}
373-
});
374-
375-
test('default plural handles consonant + y suffix (y -> ies)', function (assert) {
376-
let r = new StrictResolver({ './categories/widgets': 'widgets-cat' });
377-
378-
assert.strictEqual(r.resolve('category:widgets'), 'widgets-cat');
379-
});
380-
381-
test('default plural handles common irregular nouns', function (assert) {
382-
let cases = {
383-
'./children/alice': 'child:alice',
384-
'./people/bob': 'person:bob',
385-
'./men/carl': 'man:carl',
386-
'./women/dana': 'woman:dana',
387-
'./mice/squeaky': 'mouse:squeaky',
388-
'./teeth/molar': 'tooth:molar',
389-
'./feet/left': 'foot:left',
390-
};
391-
392-
for (let [modulePath, lookup] of Object.entries(cases)) {
393-
let r = new StrictResolver({ [modulePath]: modulePath });
394-
assert.strictEqual(r.resolve(lookup), modulePath, `${lookup} -> ${modulePath}`);
395-
}
396-
});
397-
398-
test('custom plural overrides irregular default', function (assert) {
399-
// a user who insists on "childs" should be able to opt out of the
400-
// built-in irregular plural
401-
let r = new StrictResolver({ './childs/alice': 'alice' }, { child: 'childs' });
359+
test('irregular plurals must be opted into via the plurals option', function (assert) {
360+
// Default pluralization is naive (type + 's'), matching ember-resolver's
361+
// behavior. A consumer that wants proper English irregulars registers
362+
// them up-front via the plurals map.
363+
let r = new StrictResolver(
364+
{ './children/alice': 'alice' },
365+
{ child: 'children' }
366+
);
402367

403368
assert.strictEqual(r.resolve('child:alice'), 'alice');
404369
});

smoke-tests/scenarios/scenarios.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,34 @@ function embroiderWebpack(project: Project) {
2121

2222
function embroiderVite(project: Project) {}
2323

24+
// Swap the v2-app-template's default app.js for a strict-resolver variant:
25+
// no ember-resolver, no compatModules, no modulePrefix — just a `modules =
26+
// {...import.meta.glob(...)}` literal. Making this a variant of
27+
// v2AppScenarios means every test that runs against v2AppScenarios also
28+
// runs against this configuration.
29+
function strictResolver(project: Project) {
30+
project.mergeFiles({
31+
app: {
32+
'app.js': `
33+
import Application from '@ember/application';
34+
import Router from './router';
35+
36+
export default class App extends Application {
37+
modules = {
38+
'./router': { default: Router },
39+
...import.meta.glob('./services/**/*.{js,ts}', { eager: true }),
40+
...import.meta.glob('./controllers/**/*.{js,ts}', { eager: true }),
41+
...import.meta.glob('./routes/**/*.{js,ts}', { eager: true }),
42+
...import.meta.glob('./components/**/*.{gjs,gts,js,ts}', { eager: true }),
43+
...import.meta.glob('./helpers/**/*.{js,ts}', { eager: true }),
44+
...import.meta.glob('./templates/**/*.{hbs,gjs,gts}', { eager: true }),
45+
};
46+
}
47+
`,
48+
},
49+
});
50+
}
51+
2452
export const v1AppScenarios = Scenarios.fromProject(() =>
2553
Project.fromDir(dirname(require.resolve('../app-template/package.json')), { linkDevDeps: true })
2654
).expand({
@@ -34,10 +62,9 @@ export const v2AppScenarios = Scenarios.fromProject(() =>
3462
})
3563
).expand({
3664
embroiderVite,
65+
strictResolver,
3766
});
3867

39-
function strictResolver(project: Project) {}
40-
4168
export const strictAppScenarios = Scenarios.fromProject(() =>
4269
Project.fromDir(dirname(require.resolve('../v2-app-template/package.json')), {
4370
linkDevDeps: true,

0 commit comments

Comments
 (0)