Modernize: V1 -> V2 migration - #360
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Ember CLI build/test toolchain with Vite/Rollup/Babel/Embroider and pnpm/Node 22 CI; restructures package exports/entrypoints and typings; adds demo app, module-based tests and new publish/build configs; removes legacy dummy fixtures and many legacy lint/ignore configs. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant PN as pnpm
participant Build as Vite/Rollup
participant Try as `@embroider/try`
participant Test as Testem
participant Dist as Dist Branch
Dev->>GH: push to repo
GH->>PN: pnpm install (--frozen-lockfile or --no-lockfile per job)
GH->>Try: pnpm -s dlx `@embroider/try` list (compute matrix)
GH->>Build: pnpm build / pnpm test (Vite/Rollup)
Build->>Test: produce dist-tests and invoke Testem
Test-->>GH: report results
GH->>Dist: push built package to dist branch (push-dist workflow)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
.template-lintrc.mjs (1)
3-3: Document why inlinehbstemplate-literal linting is disabled.Line 3 changes lint coverage by turning off
checkHbsTemplateLiterals; please add a short rationale so this doesn’t look like an accidental behavior regression during migration.📝 Suggested tweak
export default { extends: 'recommended', + // Disabled intentionally: <brief reason + link to issue, if applicable>. checkHbsTemplateLiterals: false, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.template-lintrc.mjs at line 3, Add a short rationale comment above the checkHbsTemplateLiterals setting explaining why inline `hbs` template-literal linting is disabled (for example: temporary migration gap, false positives from new parser, or work tracked in a specific issue/PR), reference the configuration key `checkHbsTemplateLiterals` and include a link or issue number if one exists so future reviewers know this is intentional and where to follow up..prettierignore (1)
13-16: Consider narrowing the global ignore patterns.Ignoring all
*.yaml,*.yml,*.md, and*.htmlcan unintentionally skip formatting for maintainable source files (docs/workflows/static assets). Prefer path-scoped ignores where possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.prettierignore around lines 13 - 16, The global ignore entries in .prettierignore (*.yaml, *.yml, *.md, *.html) are too broad and will skip formatting for many repo files; update .prettierignore to replace those global globs with path-scoped patterns (e.g., limit to specific directories such as docs/**, .github/workflows/**, static/**) or add explicit negation rules so only the intended generated or vendor files are ignored; edit the .prettierignore entries for the listed patterns to be directory-scoped rather than global to avoid accidentally excluding source and documentation files.unpublished-development-types/index.d.ts (1)
1-7: Avoid broadanyin publishedModelRegistryaugmentation.
[key: string]: anyweakens consumer type safety globally. Preferunknownso callers must narrow explicitly.Safer typing diff
-/* eslint-disable `@typescript-eslint/no-explicit-any` */ import 'ember-data/types/registries/model'; declare module 'ember-data/types/registries/model' { export default interface ModelRegistry { - [key: string]: any; + [key: string]: unknown; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@unpublished-development-types/index.d.ts` around lines 1 - 7, The ModelRegistry augmentation currently exposes a global index signature using any ("export default interface ModelRegistry { [key: string]: any; }"), which weakens consumer type-safety; change the index signature to use unknown instead of any so callers must explicitly narrow types (i.e., update the augmentation in index.d.ts for the ModelRegistry interface to use [key: string]: unknown). Ensure imports/exports remain the same and run type checks to catch any call-sites that now require explicit type guards or casts.tests/unit/object-validator-test.ts (1)
1-1: Narrow ESLint suppression scope.Using a file-level disable here hides unsafe usage across the whole file; prefer targeted
eslint-disable-next-lineat the specific call sites that need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/object-validator-test.ts` at line 1, Remove the file-level ESLint disables at the top of tests/unit/object-validator-test.ts and instead apply targeted eslint-disable-next-line comments only where needed: run the linter or open the file to find the specific test statements that trigger `@typescript-eslint/no-unsafe-declaration-merging`, `@typescript-eslint/no-unsafe-call`, and `@typescript-eslint/no-unsafe-member-access`, then add a single-line disable (e.g., // eslint-disable-next-line `@typescript-eslint/no-unsafe-call`) immediately above each offending expression or call; keep the rule names exactly as in the original file so reviewers can verify the scope is minimized.rollup.config.mjs (1)
33-37: KeepappReexports()aligned withember-addon.app-js.The Rollup list now glob-reexports every initializer, but
package.jsononly exposes./initializers/model-locale.jsinember-addon.app-js. Narrowing this to the same explicit paths avoids future drift between emitted_app_files and the manifest contract.♻️ Suggested change
addon.appReexports([ 'decorators/model-validator.js', 'decorators/object-validator.js', - 'initializers/**/*.js', + 'initializers/model-locale.js', ]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rollup.config.mjs` around lines 33 - 37, The appReexports call is currently globbing all initializers which drifts from the package.json ember-addon.app-js manifest that only exposes ./initializers/model-locale.js; update the addon.appReexports([...]) invocation to list the same explicit initializer path(s) (e.g., 'initializers/model-locale.js') instead of the glob 'initializers/**/*.js' so the Rollup output matches the manifest contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.development:
- Line 6: Remove the extra blank line in the environment file so there are no
consecutive empty lines; open the .env file being edited, delete the stray blank
line that triggers ExtraBlankLine, and ensure the file contains only intended
KEY=VALUE lines and ends with a single newline (no additional empty lines).
In @.github/workflows/push-dist.yml:
- Around line 23-31: Replace mutable action tags with immutable full
40-character commit SHAs for each `uses:` entry to prevent supply-chain risks:
update `actions/checkout@v4`, `pnpm/action-setup@v4`, `actions/setup-node@v4`,
and `kategengler/put-built-npm-package-contents-on-branch@v2.0.0` to their
corresponding full commit SHAs; locate those exact identifiers in the workflow
and substitute the tag suffixes with the repository@<full-sha> values, keeping
the rest of the step configuration unchanged and ensuring each SHA is the
canonical commit for the intended release.
In `@babel.config.cjs`:
- Around line 62-63: The current isCompat boolean uses
Boolean(process.env.ENABLE_COMPAT_BUILD) which treats any non-empty string
(e.g., "0" or "false") as true; update the logic that sets isCompat to
explicitly parse ENABLE_COMPAT_BUILD (e.g., normalize to lower-case and compare
against allowed truthy values like "1", "true", "yes" or parse JSON
"true"/"false") so only explicit truthy values enable the compat build; change
the assignment where isCompat is defined to perform this explicit check so
downstream uses of isCompat (and the templating/babel macros that rely on it)
behave correctly.
In `@package.json`:
- Around line 36-47: The build and prepack npm scripts currently call the
POSIX-only cp command (the "build" and "prepack" scripts), which breaks on
Windows; replace the shell copy with a small Node-based copy script and invoke
that from package.json. Create a file (e.g., scripts/copy-dts.js) that uses
node:fs or fs/promises to ensure the declarations directory exists and copies
src/index.d.ts to declarations/index.d.ts (using mkdir/copyFile or fs.cp), then
change both "build" and "prepack" to run rollup --config && node
./scripts/copy-dts.js instead of using cp so the step is cross-platform.
- Around line 15-28: The package.json is missing the "type": "module" field
causing Node to treat .js files in the "exports" (e.g., "./dist/*.js" and the
default "./dist/index.js" under ".") as CommonJS; add "type": "module" at the
top-level of package.json so Node loads the exported ESM files correctly (leave
the dedicated "./addon-main.cjs" untouched as the explicit CommonJS entry).
- Line 46: The "lint:publish" npm script is invoking an invalid command
("publint run"); update that script (the "lint:publish" entry) to call publint
correctly by replacing "publint run --level error" with "publint . --level
error" or simply "publint --level error" so publint runs on the current
directory and the --level flag is applied.
In `@tests/models/fake-model.ts`:
- Around line 78-79: The async relationship declaration for asyncModel uses the
plain type AsyncModel but should follow the established async relationship
pattern: change the property declaration decorated with
`@belongsTo`('async-model', { async: true, inverse: 'fakeModel' }) from "declare
asyncModel: AsyncModel" to use the generic async wrapper type
"AsyncBelongsTo<AsyncModel>" so it matches the AsyncBelongsTo<OtherModel>
pattern used elsewhere.
In `@tests/models/other-model.ts`:
- Around line 13-14: The declared relationship type for the property fakeModel
is incorrect: change the generic on AsyncBelongsTo from OtherModel to the actual
related model type FakeModel so it matches the `@belongsTo`('fake-model', ...)
decorator; update the declaration of the fakeModel property
(AsyncBelongsTo<OtherModel>) to AsyncBelongsTo<FakeModel> to restore correct
typing and IDE suggestions.
In `@vite.config.mjs`:
- Around line 11-16: Replace the loose truthiness check for ENABLE_COMPAT_BUILD
with an explicit string comparison: instead of using isCompat =
Boolean(process.env.ENABLE_COMPAT_BUILD), change the logic in vite.config.mjs
(and the analogous spot in babel.config.cjs) to compare
process.env.ENABLE_COMPAT_BUILD to the expected value(s) (e.g. 'true' or '1') so
only those explicit strings enable compat mode; update the isCompat variable and
any uses (like the plugins spread that references classicEmberSupport())
accordingly.
---
Nitpick comments:
In @.prettierignore:
- Around line 13-16: The global ignore entries in .prettierignore (*.yaml,
*.yml, *.md, *.html) are too broad and will skip formatting for many repo files;
update .prettierignore to replace those global globs with path-scoped patterns
(e.g., limit to specific directories such as docs/**, .github/workflows/**,
static/**) or add explicit negation rules so only the intended generated or
vendor files are ignored; edit the .prettierignore entries for the listed
patterns to be directory-scoped rather than global to avoid accidentally
excluding source and documentation files.
In @.template-lintrc.mjs:
- Line 3: Add a short rationale comment above the checkHbsTemplateLiterals
setting explaining why inline `hbs` template-literal linting is disabled (for
example: temporary migration gap, false positives from new parser, or work
tracked in a specific issue/PR), reference the configuration key
`checkHbsTemplateLiterals` and include a link or issue number if one exists so
future reviewers know this is intentional and where to follow up.
In `@rollup.config.mjs`:
- Around line 33-37: The appReexports call is currently globbing all
initializers which drifts from the package.json ember-addon.app-js manifest that
only exposes ./initializers/model-locale.js; update the
addon.appReexports([...]) invocation to list the same explicit initializer
path(s) (e.g., 'initializers/model-locale.js') instead of the glob
'initializers/**/*.js' so the Rollup output matches the manifest contract.
In `@tests/unit/object-validator-test.ts`:
- Line 1: Remove the file-level ESLint disables at the top of
tests/unit/object-validator-test.ts and instead apply targeted
eslint-disable-next-line comments only where needed: run the linter or open the
file to find the specific test statements that trigger
`@typescript-eslint/no-unsafe-declaration-merging`,
`@typescript-eslint/no-unsafe-call`, and
`@typescript-eslint/no-unsafe-member-access`, then add a single-line disable
(e.g., // eslint-disable-next-line `@typescript-eslint/no-unsafe-call`)
immediately above each offending expression or call; keep the rule names exactly
as in the original file so reviewers can verify the scope is minimized.
In `@unpublished-development-types/index.d.ts`:
- Around line 1-7: The ModelRegistry augmentation currently exposes a global
index signature using any ("export default interface ModelRegistry { [key:
string]: any; }"), which weakens consumer type-safety; change the index
signature to use unknown instead of any so callers must explicitly narrow types
(i.e., update the augmentation in index.d.ts for the ModelRegistry interface to
use [key: string]: unknown). Ensure imports/exports remain the same and run type
checks to catch any call-sites that now require explicit type guards or casts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf916a75-64ea-43c9-b84b-06236b8201d9
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/.DS_Storeis excluded by!**/.DS_Storetests/dummy/.DS_Storeis excluded by!**/.DS_Storeyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (92)
.ember-cli.env.development.eslintignore.eslintrc.js.github/dependabot.yml.github/workflows/ci.yml.github/workflows/push-dist.yml.gitignore.node-version.npmignore.npmrc.nvmrc.prettierignore.prettierrc.prettierrc.js.prettierrc.mjs.template-lintrc.js.template-lintrc.mjs.tool-versions.try.mjs.watchmanconfigaddon-main.cjsaddon/index.jsapp/decorators/model-validator.jsapp/decorators/object-validator.jsapp/initializers/model-locale.jsbabel.config.cjsbabel.publish.config.cjsconfig/ember-cli-update.jsondemo-app/app.gtsdemo-app/styles.cssdemo-app/templates/application.gtsember-cli-build.jseslint.config.mjsindex.htmlindex.jspackage.jsonrollup.config.mjssrc/decorators/core-validator.jssrc/decorators/model-validator.jssrc/decorators/object-validator.jssrc/index.d.tssrc/index.jssrc/initializers/model-locale.tssrc/messages/ar.tssrc/messages/en.tssrc/messages/es.tssrc/messages/fr.tssrc/messages/hu.tssrc/messages/pt-br.tssrc/messages/sr-cyrl.tssrc/messages/sr.tssrc/messages/tr.tssrc/messages/uk.tssrc/postal-codes-regex.tstestem.cjstestem.jstests/adapters/application.tstests/dummy/app/app.tstests/dummy/app/components/.gitkeeptests/dummy/app/config/environment.d.tstests/dummy/app/controllers/.gitkeeptests/dummy/app/helpers/.gitkeeptests/dummy/app/index.htmltests/dummy/app/router.tstests/dummy/app/routes/.gitkeeptests/dummy/app/styles/app.csstests/dummy/app/templates/application.hbstests/dummy/config/ember-cli-update.jsontests/dummy/config/ember-try.jstests/dummy/config/environment.jstests/dummy/config/optional-features.jsontests/dummy/config/targets.jstests/dummy/public/robots.txttests/helpers/index.tstests/helpers/message-formater.jstests/index.htmltests/integration/.gitkeeptests/models/async-model.tstests/models/fake-model.tstests/models/other-model.tstests/test-helper.jstests/test-helper.tstests/unit/models/fake-model-test.jstests/unit/object-validator-test.tstsconfig.jsontsconfig.publish.jsontypes/dummy/index.d.tstypes/ember-data/types/registries/model.d.tstypes/global.d.tsunpublished-development-types/index.d.tsvite.config.mjs
💤 Files with no reviewable changes (34)
- .github/dependabot.yml
- .nvmrc
- .prettierrc
- .tool-versions
- .watchmanconfig
- .ember-cli
- .node-version
- .template-lintrc.js
- tests/dummy/public/robots.txt
- app/decorators/model-validator.js
- tests/dummy/config/ember-cli-update.json
- index.js
- .prettierrc.js
- tests/dummy/config/optional-features.json
- app/decorators/object-validator.js
- app/initializers/model-locale.js
- .npmignore
- types/ember-data/types/registries/model.d.ts
- addon/index.js
- tests/test-helper.ts
- .eslintignore
- tests/dummy/app/app.ts
- tests/dummy/app/templates/application.hbs
- ember-cli-build.js
- types/global.d.ts
- .eslintrc.js
- tests/dummy/app/index.html
- tests/dummy/config/targets.js
- tests/dummy/app/router.ts
- tests/dummy/config/environment.js
- tests/dummy/config/ember-try.js
- tests/helpers/index.ts
- tests/dummy/app/config/environment.d.ts
- testem.js
| # Vite recommends using .env.local or .env.[mode].local if you need to manage secrets | ||
| # SEE: https://vite.dev/guide/env-and-mode.html#env-files for more information. | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank line to satisfy dotenv-linter.
Line 6 triggers ExtraBlankLine.
Proposed cleanup
# SEE: https://vite.dev/guide/env-and-mode.html#env-files for more information.
-
# Default NODE_ENV with vite build --mode=test is production
NODE_ENV=development📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # SEE: https://vite.dev/guide/env-and-mode.html#env-files for more information. | |
| # Default NODE_ENV with vite build --mode=test is production | |
| NODE_ENV=development |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 6-6: [ExtraBlankLine] Extra blank line detected
(ExtraBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.development at line 6, Remove the extra blank line in the environment
file so there are no consecutive empty lines; open the .env file being edited,
delete the stray blank line that triggers ExtraBlankLine, and ensure the file
contains only intended KEY=VALUE lines and ends with a single newline (no
additional empty lines).
| - uses: actions/checkout@v4 | ||
| - uses: pnpm/action-setup@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22 | ||
| cache: pnpm | ||
| - name: Install Dependencies | ||
| run: pnpm install --frozen-lockfile | ||
| - uses: kategengler/put-built-npm-package-contents-on-branch@v2.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List workflow actions not pinned to a 40-char commit SHA
rg -n "^\s*-\s*uses:\s*[^[:space:]]+@[^[:space:]]+" .github/workflows -g '*.yml' -g '*.yaml' \
| awk '{
if ($0 !~ /@[0-9a-fA-F]{40}$/) print
}'Repository: esbanarango/ember-model-validator
Length of output: 962
Pin workflow actions to full commit SHAs.
All uses: entries at lines 23-31 use mutable tags (@v4, @v2.0.0) instead of immutable commit SHAs. Since this workflow has write permissions (contents: write), pinning to full 40-character commit hashes is necessary to prevent supply-chain attacks via tag retargeting.
Change:
actions/checkout@v4→actions/checkout@<full-sha>pnpm/action-setup@v4→pnpm/action-setup@<full-sha>actions/setup-node@v4→actions/setup-node@<full-sha>kategengler/put-built-npm-package-contents-on-branch@v2.0.0→kategengler/put-built-npm-package-contents-on-branch@<full-sha>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/push-dist.yml around lines 23 - 31, Replace mutable action
tags with immutable full 40-character commit SHAs for each `uses:` entry to
prevent supply-chain risks: update `actions/checkout@v4`,
`pnpm/action-setup@v4`, `actions/setup-node@v4`, and
`kategengler/put-built-npm-package-contents-on-branch@v2.0.0` to their
corresponding full commit SHAs; locate those exact identifiers in the workflow
and substitute the tag suffixes with the repository@<full-sha> values, keeping
the rest of the step configuration unchanged and ensuring each SHA is the
canonical commit for the intended release.
| // For scenario testing | ||
| const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'NODE'
for (const raw of [undefined, '', '0', '1', 'false', 'true']) {
console.log(`${String(raw).padEnd(9)} => ${Boolean(raw)}`);
}
NODERepository: esbanarango/ember-model-validator
Length of output: 186
🏁 Script executed:
cd esbanarango/ember-model-validator && cat -n babel.config.cjs | sed -n '55,75p'Repository: esbanarango/ember-model-validator
Length of output: 160
🏁 Script executed:
fd -name babel.config.cjsRepository: esbanarango/ember-model-validator
Length of output: 307
🏁 Script executed:
fd babel.config.cjsRepository: esbanarango/ember-model-validator
Length of output: 93
🏁 Script executed:
cat -n babel.config.cjs | sed -n '55,75p'Repository: esbanarango/ember-model-validator
Length of output: 636
🏁 Script executed:
cat -n babel.config.cjs | grep -A 20 'const isCompat'Repository: esbanarango/ember-model-validator
Length of output: 682
🏁 Script executed:
rg 'isCompat' babel.config.cjsRepository: esbanarango/ember-model-validator
Length of output: 273
Parse ENABLE_COMPAT_BUILD explicitly.
Line 63 treats any non-empty string as enabled, so "0" and "false" still select the compat pipeline. This affects which template and babel macros are used (lines 79+).
Suggested change
-const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD);
+const isCompat = /^(1|true)$/i.test(process.env.ENABLE_COMPAT_BUILD ?? '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For scenario testing | |
| const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD); | |
| // For scenario testing | |
| const isCompat = /^(1|true)$/i.test(process.env.ENABLE_COMPAT_BUILD ?? ''); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@babel.config.cjs` around lines 62 - 63, The current isCompat boolean uses
Boolean(process.env.ENABLE_COMPAT_BUILD) which treats any non-empty string
(e.g., "0" or "false") as true; update the logic that sets isCompat to
explicitly parse ENABLE_COMPAT_BUILD (e.g., normalize to lower-case and compare
against allowed truthy values like "1", "true", "yes" or parse JSON
"true"/"false") so only explicit truthy values enable the compat build; change
the assignment where isCompat is defined to perform this explicit check so
downstream uses of isCompat (and the templating/babel macros that rely on it)
behave correctly.
| "imports": { | ||
| "#src/*": "./src/*" | ||
| }, | ||
| "exports": { | ||
| ".": { | ||
| "types": "./declarations/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| }, | ||
| "./addon-main.js": "./addon-main.cjs", | ||
| "./*": { | ||
| "types": "./declarations/*.d.ts", | ||
| "default": "./dist/*.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For a Node package that exports ./dist/*.jsESM files, doespackage.jsonneed"type": "module"when those files are not.mjs? How are exported .jsfiles interpreted whentype is missing?
💡 Result:
Yes, for a Node.js package that exports ./dist/*.js files containing ESM syntax (e.g., import/export statements), the package.json must include "type": "module" to ensure those .js files are interpreted as ES modules. Without "type": "module", .js files default to CommonJS interpretation. When "type" is missing in package.json, Node.js treats all .js files in that package scope as CommonJS modules. Attempting to load ESM syntax in such files will throw a SyntaxError, as CommonJS does not support import/export. This behavior is consistent across Node.js versions since v12.0.0, including the latest v25.x, and applies to files referenced via "exports", "main", or direct imports.
Citations:
- 1: https://nodejs.org/api/packages.html
- 2: https://r2.nodejs.org/docs/v21.0.0/api/packages.html
- 3: https://nodejs.org/dist/latest-v19.x/docs/api/packages.html
- 4: https://nodejs.org/docs/latest/api/esm.html
Add "type": "module" to mark the package as ESM.
The package exports ./dist/*.js files containing ESM syntax, but lacks the "type": "module" declaration. Without it, Node.js interprets .js files as CommonJS by default, causing runtime SyntaxErrors when ESM code is loaded. The dedicated .cjs extension for addon-main.cjs confirms the intent to use ESM for the main export; the declaration must match.
Suggested change
"license": "MIT",
"author": "Esteban Arango Medina",
+ "type": "module",
"imports": {
"#src/*": "./src/*"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 15 - 28, The package.json is missing the "type":
"module" field causing Node to treat .js files in the "exports" (e.g.,
"./dist/*.js" and the default "./dist/index.js" under ".") as CommonJS; add
"type": "module" at the top-level of package.json so Node loads the exported ESM
files correctly (leave the dedicated "./addon-main.cjs" untouched as the
explicit CommonJS entry).
| "build": "rollup --config && cp src/index.d.ts declarations/index.d.ts", | ||
| "format": "prettier . --cache --write", | ||
| "lint": "concurrently \"pnpm:lint:*(!fix)\" --names \"lint:\" --prefixColors auto", | ||
| "lint:fix": "concurrently \"pnpm:lint:*:fix\" --names \"fix:\" --prefixColors auto && pnpm run format", | ||
| "lint:format": "prettier . --cache --check", | ||
| "lint:hbs": "ember-template-lint . --no-error-on-unmatched-pattern", | ||
| "lint:hbs:fix": "ember-template-lint . --fix --no-error-on-unmatched-pattern", | ||
| "lint:js": "eslint . --cache", | ||
| "lint:js:fix": "eslint . --fix", | ||
| "lint:types": "tsc --noEmit", | ||
| "start": "ember serve", | ||
| "test": "concurrently \"npm:lint\" \"npm:test:*\" --names \"lint,test:\"", | ||
| "test:ember": "ember test", | ||
| "test:ember-compatibility": "ember try:each", | ||
| "release": "release-it", | ||
| "prepack": "ember ts:precompile", | ||
| "postpack": "ember ts:clean" | ||
| "lint:types": "ember-tsc --noEmit", | ||
| "lint:publish": "pnpm build && publint run --level error", | ||
| "prepack": "rollup --config && cp src/index.d.ts declarations/index.d.ts", |
There was a problem hiding this comment.
Use a cross-platform copy step in build and prepack.
Line 36 and Line 47 now depend on POSIX cp, so packaging breaks for Windows contributors/publishers. Since this repo already targets modern Node, a node:fs copy keeps the scripts portable.
🛠️ Suggested change
- "build": "rollup --config && cp src/index.d.ts declarations/index.d.ts",
+ "build": "rollup --config && node -e \"const fs=require('node:fs'); fs.mkdirSync('declarations', { recursive: true }); fs.copyFileSync('src/index.d.ts', 'declarations/index.d.ts')\"",
@@
- "prepack": "rollup --config && cp src/index.d.ts declarations/index.d.ts",
+ "prepack": "rollup --config && node -e \"const fs=require('node:fs'); fs.mkdirSync('declarations', { recursive: true }); fs.copyFileSync('src/index.d.ts', 'declarations/index.d.ts')\"",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "build": "rollup --config && cp src/index.d.ts declarations/index.d.ts", | |
| "format": "prettier . --cache --write", | |
| "lint": "concurrently \"pnpm:lint:*(!fix)\" --names \"lint:\" --prefixColors auto", | |
| "lint:fix": "concurrently \"pnpm:lint:*:fix\" --names \"fix:\" --prefixColors auto && pnpm run format", | |
| "lint:format": "prettier . --cache --check", | |
| "lint:hbs": "ember-template-lint . --no-error-on-unmatched-pattern", | |
| "lint:hbs:fix": "ember-template-lint . --fix --no-error-on-unmatched-pattern", | |
| "lint:js": "eslint . --cache", | |
| "lint:js:fix": "eslint . --fix", | |
| "lint:types": "tsc --noEmit", | |
| "start": "ember serve", | |
| "test": "concurrently \"npm:lint\" \"npm:test:*\" --names \"lint,test:\"", | |
| "test:ember": "ember test", | |
| "test:ember-compatibility": "ember try:each", | |
| "release": "release-it", | |
| "prepack": "ember ts:precompile", | |
| "postpack": "ember ts:clean" | |
| "lint:types": "ember-tsc --noEmit", | |
| "lint:publish": "pnpm build && publint run --level error", | |
| "prepack": "rollup --config && cp src/index.d.ts declarations/index.d.ts", | |
| "build": "rollup --config && node -e \"const fs=require('node:fs'); fs.mkdirSync('declarations', { recursive: true }); fs.copyFileSync('src/index.d.ts', 'declarations/index.d.ts')\"", | |
| "format": "prettier . --cache --write", | |
| "lint": "concurrently \"pnpm:lint:*(!fix)\" --names \"lint:\" --prefixColors auto", | |
| "lint:fix": "concurrently \"pnpm:lint:*:fix\" --names \"fix:\" --prefixColors auto && pnpm run format", | |
| "lint:format": "prettier . --cache --check", | |
| "lint:hbs": "ember-template-lint . --no-error-on-unmatched-pattern", | |
| "lint:hbs:fix": "ember-template-lint . --fix --no-error-on-unmatched-pattern", | |
| "lint:js": "eslint . --cache", | |
| "lint:js:fix": "eslint . --fix", | |
| "lint:types": "ember-tsc --noEmit", | |
| "lint:publish": "pnpm build && publint run --level error", | |
| "prepack": "rollup --config && node -e \"const fs=require('node:fs'); fs.mkdirSync('declarations', { recursive: true }); fs.copyFileSync('src/index.d.ts', 'declarations/index.d.ts')\"", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 36 - 47, The build and prepack npm scripts
currently call the POSIX-only cp command (the "build" and "prepack" scripts),
which breaks on Windows; replace the shell copy with a small Node-based copy
script and invoke that from package.json. Create a file (e.g.,
scripts/copy-dts.js) that uses node:fs or fs/promises to ensure the declarations
directory exists and copies src/index.d.ts to declarations/index.d.ts (using
mkdir/copyFile or fs.cp), then change both "build" and "prepack" to run rollup
--config && node ./scripts/copy-dts.js instead of using cp so the step is
cross-platform.
| "prepack": "ember ts:precompile", | ||
| "postpack": "ember ts:clean" | ||
| "lint:types": "ember-tsc --noEmit", | ||
| "lint:publish": "pnpm build && publint run --level error", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the publint CLI syntax for version 0.3.16? Does it support publint run --level error, or should the command be publint --level error/publint . --level error?
💡 Result:
The publint CLI syntax for version 0.3.16 is publint [path] [options], where [path] is an optional directory path or tarball (defaults to current directory), and options include --level, --pack, and --strict. It does not support publint run --level error. The correct commands are publint --level error (current directory) or publint . --level error (explicit current directory).
Citations:
- 1: https://publint.dev/docs/cli
- 2: https://github.com/publint/publint/blob/master/packages/publint/CHANGELOG.md
- 3: https://github.com/publint/publint/releases
🏁 Script executed:
cat package.json | grep -A 5 -B 5 "publint"Repository: esbanarango/ember-model-validator
Length of output: 977
Fix publint invocation syntax.
publint run is not a supported command. Line 46 should use publint . --level error (or publint --level error for the current directory) instead.
Suggested change
- "lint:publish": "pnpm build && publint run --level error",
+ "lint:publish": "pnpm build && publint . --level error",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 46, The "lint:publish" npm script is invoking an
invalid command ("publint run"); update that script (the "lint:publish" entry)
to call publint correctly by replacing "publint run --level error" with "publint
. --level error" or simply "publint --level error" so publint runs on the
current directory and the --level flag is applied.
| @belongsTo('async-model', { async: true, inverse: 'fakeModel' }) | ||
| declare asyncModel: AsyncModel; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify async belongsTo typing patterns in model fixtures (read-only).
rg -n --type=ts -C2 "@belongsTo\\(.*async:\\s*true" tests/models
rg -n --type=ts -C2 "declare\\s+\\w+\\s*:\\s*AsyncBelongsTo<" tests/modelsRepository: esbanarango/ember-model-validator
Length of output: 1690
🏁 Script executed:
head -30 tests/models/fake-model.tsRepository: esbanarango/ember-model-validator
Length of output: 1125
Use AsyncBelongsTo<AsyncModel> for the async belongsTo field.
async: true with declare asyncModel: AsyncModel is inconsistent with the established async relationship typing pattern elsewhere in this file. Line 77 already correctly uses AsyncBelongsTo<OtherModel>.
Proposed fix
`@belongsTo`('async-model', { async: true, inverse: 'fakeModel' })
- declare asyncModel: AsyncModel;
+ declare asyncModel: AsyncBelongsTo<AsyncModel>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @belongsTo('async-model', { async: true, inverse: 'fakeModel' }) | |
| declare asyncModel: AsyncModel; | |
| `@belongsTo`('async-model', { async: true, inverse: 'fakeModel' }) | |
| declare asyncModel: AsyncBelongsTo<AsyncModel>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/models/fake-model.ts` around lines 78 - 79, The async relationship
declaration for asyncModel uses the plain type AsyncModel but should follow the
established async relationship pattern: change the property declaration
decorated with `@belongsTo`('async-model', { async: true, inverse: 'fakeModel' })
from "declare asyncModel: AsyncModel" to use the generic async wrapper type
"AsyncBelongsTo<AsyncModel>" so it matches the AsyncBelongsTo<OtherModel>
pattern used elsewhere.
| @belongsTo('fake-model', { async: true, inverse: 'otherFakes' }) | ||
| declare fakeModel: AsyncBelongsTo<OtherModel>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file mentioned in the review
find . -name "other-model.ts" -type fRepository: esbanarango/ember-model-validator
Length of output: 106
🏁 Script executed:
# Also search for any model files in tests directory
find ./tests -name "*.ts" -type f | head -20Repository: esbanarango/ember-model-validator
Length of output: 235
🏁 Script executed:
# Read the other-model.ts file to see the exact code
cat -n ./tests/models/other-model.tsRepository: esbanarango/ember-model-validator
Length of output: 1151
🏁 Script executed:
# Also read fake-model.ts to understand the related model structure
cat -n ./tests/models/fake-model.tsRepository: esbanarango/ember-model-validator
Length of output: 11501
Fix the related model type for fakeModel.
Line 14 declares the type as AsyncBelongsTo<OtherModel>, but the decorator on line 13 targets 'fake-model'. The generic type should match the related model—it should be AsyncBelongsTo<FakeModel>. This ensures type-safe access to the relationship and prevents misleading IDE suggestions.
Suggested fix
import Model, { attr, belongsTo, type AsyncBelongsTo } from '@ember-data/model';
+import type FakeModel from './fake-model';
import {
modelValidator,
type ValidationsConfig,
type ValidatedModel,
} from 'ember-model-validator';
interface OtherModel extends ValidatedModel, Model {}
`@modelValidator`
class OtherModel extends Model {
`@belongsTo`('fake-model', { async: true, inverse: 'otherFakes' })
- declare fakeModel: AsyncBelongsTo<OtherModel>;
+ declare fakeModel: AsyncBelongsTo<FakeModel>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/models/other-model.ts` around lines 13 - 14, The declared relationship
type for the property fakeModel is incorrect: change the generic on
AsyncBelongsTo from OtherModel to the actual related model type FakeModel so it
matches the `@belongsTo`('fake-model', ...) decorator; update the declaration of
the fakeModel property (AsyncBelongsTo<OtherModel>) to AsyncBelongsTo<FakeModel>
to restore correct typing and IDE suggestions.
| const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD); | ||
|
|
||
| export default defineConfig({ | ||
| plugins: [ | ||
| ...(isCompat ? [classicEmberSupport()] : []), | ||
| ember(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect all assignments/usages to confirm actual env values used in CI/scenarios
rg -n "ENABLE_COMPAT_BUILD" -C2Repository: esbanarango/ember-model-validator
Length of output: 492
🏁 Script executed:
# Search for CI/workflow configurations
fd -t f -e yml -e yaml . 2>/dev/null | head -20Repository: esbanarango/ember-model-validator
Length of output: 91
🏁 Script executed:
# Search for any documentation or comments about ENABLE_COMPAT_BUILD
rg -i "compat" -t md -t txt --max-count 20Repository: esbanarango/ember-model-validator
Length of output: 149
🏁 Script executed:
# Check package.json scripts to see how build/test commands use this variable
cat package.json | head -80Repository: esbanarango/ember-model-validator
Length of output: 2808
🏁 Script executed:
# Search for CI/workflow files more thoroughly
find . -name ".github" -o -name ".circleci" -o -name "*.yml" -o -name "*.yaml" 2>/dev/null | grep -E "\.(yml|yaml)$|workflows" | head -20Repository: esbanarango/ember-model-validator
Length of output: 1776
🏁 Script executed:
# Examine vite.config.mjs to see how isCompat is actually used
cat vite.config.mjsRepository: esbanarango/ember-model-validator
Length of output: 659
🏁 Script executed:
# Examine babel.config.cjs to see how isCompat is actually used
cat babel.config.cjsRepository: esbanarango/ember-model-validator
Length of output: 2676
🏁 Script executed:
# Check the CI workflow to see how ENABLE_COMPAT_BUILD is used
cat .github/workflows/ci.ymlRepository: esbanarango/ember-model-validator
Length of output: 2017
🏁 Script executed:
# Check testem config
cat testem.cjs 2>/dev/null || echo "testem.cjs not found"Repository: esbanarango/ember-model-validator
Length of output: 773
🏁 Script executed:
# Search for any explicit setting of ENABLE_COMPAT_BUILD
rg "ENABLE_COMPAT_BUILD\s*=" -B2 -A2Repository: esbanarango/ember-model-validator
Length of output: 59
🏁 Script executed:
# Check if there's any documentation about the variable or how scenarios work
rg -i "scenario|compat.*build" --max-count 30Repository: esbanarango/ember-model-validator
Length of output: 566
Use explicit string comparison for ENABLE_COMPAT_BUILD environment variable.
The current code uses Boolean(process.env.ENABLE_COMPAT_BUILD), which treats any non-empty string (including "false" or "0") as true. This can cause compat builds to be unintentionally enabled in scenario testing.
Suggested fix
-const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD);
+const compatFlag = (process.env.ENABLE_COMPAT_BUILD ?? '').toLowerCase();
+const isCompat = compatFlag === 'true' || compatFlag === '1';This applies to both vite.config.mjs and babel.config.cjs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD); | |
| export default defineConfig({ | |
| plugins: [ | |
| ...(isCompat ? [classicEmberSupport()] : []), | |
| ember(), | |
| const compatFlag = (process.env.ENABLE_COMPAT_BUILD ?? '').toLowerCase(); | |
| const isCompat = compatFlag === 'true' || compatFlag === '1'; | |
| export default defineConfig({ | |
| plugins: [ | |
| ...(isCompat ? [classicEmberSupport()] : []), | |
| ember(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.mjs` around lines 11 - 16, Replace the loose truthiness check for
ENABLE_COMPAT_BUILD with an explicit string comparison: instead of using
isCompat = Boolean(process.env.ENABLE_COMPAT_BUILD), change the logic in
vite.config.mjs (and the analogous spot in babel.config.cjs) to compare
process.env.ENABLE_COMPAT_BUILD to the expected value(s) (e.g. 'true' or '1') so
only those explicit strings enable compat mode; update the isCompat variable and
any uses (like the plugins spread that references classicEmberSupport())
accordingly.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
package.json (3)
15-28:⚠️ Potential issue | 🟠 MajorAdd
"type": "module"for the published ESM.jsexports.The export map points consumers to
./dist/*.js; without top-level"type": "module", Node may load those package-scoped.jsfiles as CommonJS. Keepaddon-main.cjsas the explicit CJS entry. See the Node package docs on thetypefield.Suggested fix
"license": "MIT", "author": "Esteban Arango Medina", + "type": "module", "imports": { "#src/*": "./src/*" },Read-only verification
#!/bin/bash # Verifies whether package.json declares the package module type expected by .js ESM exports. node - <<'NODE' const fs = require('node:fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); const runtimeTargets = JSON.stringify(pkg.exports ?? {}); console.log(`package type: ${pkg.type ?? '<missing>'}`); console.log(`exports: ${runtimeTargets}`); if (pkg.type !== 'module' && runtimeTargets.includes('./dist/') && runtimeTargets.includes('.js')) { console.error('Missing "type": "module" while exporting ./dist/*.js runtime files.'); process.exitCode = 1; } NODEExpected result before the fix: reports the missing
"type": "module"condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 15 - 28, The package.json is missing a top-level "type": "module" while the exports map exposes ESM files under "./dist/*.js"; add "type": "module" to package.json at the top level so Node treats .js files in the package as ESM, and leave the explicit CommonJS entry "./addon-main.cjs" unchanged to preserve its CJS semantics.
36-36:⚠️ Potential issue | 🟠 MajorUse a cross-platform declaration copy step.
cpmakesbuildandprepackshell-dependent. A tiny Node helper keeps packaging portable.Suggested fix
- "build": "rollup --config && cp src/index.d.ts declarations/index.d.ts", + "build": "rollup --config && node ./scripts/copy-declarations.cjs", @@ - "prepack": "rollup --config && cp src/index.d.ts declarations/index.d.ts", + "prepack": "rollup --config && node ./scripts/copy-declarations.cjs",Add the helper:
// scripts/copy-declarations.cjs const fs = require('node:fs'); fs.mkdirSync('declarations', { recursive: true }); fs.copyFileSync('src/index.d.ts', 'declarations/index.d.ts');Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 36, The package.json "build" script uses a shell-only cp command (and similarly in the "prepack" script) which breaks cross-platform builds; create a small Node helper named copy-declarations.cjs that makes the declarations directory and copies src/index.d.ts to declarations/index.d.ts, then update the "build" and "prepack" npm scripts to invoke that helper with node (e.g., run rollup --config && node copy-declarations.cjs) instead of cp so the copy step is platform-independent.
46-46:⚠️ Potential issue | 🟠 MajorFix the
publintinvocation.
publint runis not a supported CLI form; usepublint . --level errororpublint --level error. See the publint CLI docs.Suggested fix
- "lint:publish": "pnpm build && publint run --level error", + "lint:publish": "pnpm build && publint . --level error",Read-only verification
#!/bin/bash # Verifies that lint:publish is not using the unsupported `publint run` form. node - <<'NODE' const fs = require('node:fs'); const pkg = JSON.parse(fs.readFileSync('package.json', 'utf8')); const script = pkg.scripts?.['lint:publish'] ?? ''; console.log(`lint:publish: ${script}`); if (/\bpublint\s+run\b/.test(script)) { console.error('Invalid publint invocation: replace `publint run` with `publint .` or `publint`.'); process.exitCode = 1; } NODEExpected result before the fix: reports the invalid
publint runinvocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 46, The package.json script "lint:publish" currently uses an unsupported CLI form `publint run`; update the script value for "lint:publish" to call publint correctly (e.g., replace `publint run --level error` with `publint . --level error` or `publint --level error`) so the command becomes something like `pnpm build && publint . --level error`; edit the "lint:publish" entry in package.json (look for the "lint:publish" script) and save the corrected command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package.json`:
- Around line 15-28: The package.json is missing a top-level "type": "module"
while the exports map exposes ESM files under "./dist/*.js"; add "type":
"module" to package.json at the top level so Node treats .js files in the
package as ESM, and leave the explicit CommonJS entry "./addon-main.cjs"
unchanged to preserve its CJS semantics.
- Line 36: The package.json "build" script uses a shell-only cp command (and
similarly in the "prepack" script) which breaks cross-platform builds; create a
small Node helper named copy-declarations.cjs that makes the declarations
directory and copies src/index.d.ts to declarations/index.d.ts, then update the
"build" and "prepack" npm scripts to invoke that helper with node (e.g., run
rollup --config && node copy-declarations.cjs) instead of cp so the copy step is
platform-independent.
- Line 46: The package.json script "lint:publish" currently uses an unsupported
CLI form `publint run`; update the script value for "lint:publish" to call
publint correctly (e.g., replace `publint run --level error` with `publint .
--level error` or `publint --level error`) so the command becomes something like
`pnpm build && publint . --level error`; edit the "lint:publish" entry in
package.json (look for the "lint:publish" script) and save the corrected
command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c45b3fd2-bf37-4efa-817d-e05fb2340784
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
babel.config.cjspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- babel.config.cjs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.try.mjs (2)
27-105: Optional: DRY the six LTS scenario entries.The six
ember-lts-*scenarios are identical except for theember-sourcerange. Consider generating them from a list to reduce copy-paste drift (e.g., ifcompatDepsorenvneeds to change later, it has to be updated in six places today).♻️ Example refactor
+const ltsVersions = ['4.4.0', '4.8.0', '4.12.0', '5.4.0', '5.8.0', '5.12.0']; +const ltsScenarios = ltsVersions.map((v) => ({ + name: `ember-lts-${v.split('.').slice(0, 2).join('.')}`, + npm: { + devDependencies: { + 'ember-source': `~${v}`, + ...compatDeps, + }, + }, + env: { ENABLE_COMPAT_BUILD: true }, + files: compatFiles, +})); + export default { scenarios: [ - { name: 'ember-lts-4.4', /* ... */ }, - /* ...five more nearly-identical entries... */ + ...ltsScenarios, { name: 'ember-lts-6.4', /* ... */ }, /* channels */ ], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.try.mjs around lines 27 - 105, The six repeated scenario objects in the scenarios array (the entries named 'ember-lts-4.4' ... 'ember-lts-5.12') only differ by the ember-source range; refactor by creating a list of versions (e.g., ['~4.4.0','~4.8.0', ...]) and map over it to programmatically build each scenario, reusing compatDeps, compatFiles and the env { ENABLE_COMPAT_BUILD: true } so changes to compatDeps/env/files only need to be made once; locate the scenarios array and replace the six hard-coded objects with a generated list that composes name ('ember-lts-X') and npm.devDependencies['ember-source'] from each version.
36-38: Environment variable stringification creates a footgun iffalseis ever added.The boolean
trueis correctly handled—it stringifies to"true", whichBoolean()coerces totrue. However, if someone later tries usingfalseto disable the flag (e.g., in a new scenario),Boolean("false")would still evaluate totruesince non-empty strings are truthy. Currently, disabled scenarios simply omit the variable, so this isn't an immediate issue. Consider using string values like'1'for enabled and''for disabled, or document that only omitting the variable disables the feature, to avoid this pitfall for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.try.mjs around lines 36 - 38, The env block sets ENABLE_COMPAT_BUILD to a boolean which gets stringified, causing future false -> "false" to be truthy; update the env assignment for ENABLE_COMPAT_BUILD in the .try.mjs config so it uses an explicit string-based flag (e.g., "1" for enabled and ""/omit for disabled) or ensure the variable is omitted when disabled and document that behavior; locate the env object where ENABLE_COMPAT_BUILD is defined and replace the boolean usage with the chosen string convention and add a short comment describing the convention for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.try.mjs:
- Around line 27-105: The six repeated scenario objects in the scenarios array
(the entries named 'ember-lts-4.4' ... 'ember-lts-5.12') only differ by the
ember-source range; refactor by creating a list of versions (e.g.,
['~4.4.0','~4.8.0', ...]) and map over it to programmatically build each
scenario, reusing compatDeps, compatFiles and the env { ENABLE_COMPAT_BUILD:
true } so changes to compatDeps/env/files only need to be made once; locate the
scenarios array and replace the six hard-coded objects with a generated list
that composes name ('ember-lts-X') and npm.devDependencies['ember-source'] from
each version.
- Around line 36-38: The env block sets ENABLE_COMPAT_BUILD to a boolean which
gets stringified, causing future false -> "false" to be truthy; update the env
assignment for ENABLE_COMPAT_BUILD in the .try.mjs config so it uses an explicit
string-based flag (e.g., "1" for enabled and ""/omit for disabled) or ensure the
variable is omitted when disabled and document that behavior; locate the env
object where ENABLE_COMPAT_BUILD is defined and replace the boolean usage with
the chosen string convention and add a short comment describing the convention
for future maintainers.
69327e9 to
c633872
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.try.mjs (2)
3-9: Heads-up: embeddedember-cli-build.cjsis a string, so it won’t be linted/formatted.Since this CJS module is injected as a template literal, ESLint/Prettier/type-checks will silently skip it and any syntax regression will only surface when a try scenario runs in CI. Consider either (a) keeping a real
ember-cli-build.cjsfixture on disk and reading it viafs.readFileSyncin this file, or (b) adding a minimal smoke test thatnew Function(...)-parses the string at module load. Low priority, but it removes a class of silent breakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.try.mjs around lines 3 - 9, The embedded "ember-cli-build.cjs" template literal is not linted or parsed, so either replace the inline string with reading a real fixture file (use fs.readFileSync to load the on-disk ember-cli-build.cjs content and use that string instead of the template literal) or add a minimal smoke parse at module load that attempts to parse the template with new Function(<template>) (or similar) and throws/logs on SyntaxError; update the code paths that reference the template literal (the ember-cli-build.cjs value in this file) to use the loaded/validated string so syntax regressions fail early.
19-124: Optional: DRY up the repeated compat scenarios.The seven compat scenarios (
ember-lts-3.28throughember-lts-5.12) differ only byember-sourceversion and a couple of one-off tweaks (extraember-cli/ember-page-titlepins on 3.28, presence ofSKIP_DECLARATIONSon the 3.28–4.12 block). A small helper would make intent clearer and reduce drift risk when versions are bumped.♻️ Sketch
+const compatScenario = (name, emberSource, { extraDeps = {}, skipDeclarations = false } = {}) => ({ + name, + npm: { + devDependencies: { 'ember-source': emberSource, ...compatDeps, ...extraDeps }, + }, + env: { + ENABLE_COMPAT_BUILD: true, + ...(skipDeclarations ? { SKIP_DECLARATIONS: true } : {}), + }, + files: compatFiles, +});Then scenarios become one-liners, e.g.
compatScenario('ember-lts-4.4', '~4.4.0', { skipDeclarations: true }).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.try.mjs around lines 19 - 124, The scenarios block repeats nearly identical compat scenarios (ember-lts-3.28 → ember-lts-5.12) differing only by ember-source and a couple flags; factor this into a small helper to DRY it. Add a function (e.g., compatScenario(name, emberSourceVersion, opts = {})) near the top that returns the scenario object using existing symbols compatDeps and compatFiles, accept options for skipDeclarations and extraPins (to inject the ember-cli/ember-page-title pins for 'ember-lts-3.28'), then replace the seven inline objects in the scenarios array with calls to compatScenario('ember-lts-3.28','~3.28.0',{skipDeclarations:true, extraPins:{'ember-cli':'^4.12.0','ember-page-title':'~8.2.4'}}) and similar calls for 4.4/4.8/4.12/5.4/5.8/5.12 (use skipDeclarations:true for 3.28–4.12), preserving env keys like ENABLE_COMPAT_BUILD and SKIP_DECLARATIONS when requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 49: Update the "test" npm script in package.json to use Vite's camelCase
flag: change the Vite build option from --out-dir to --outDir so the "test"
script (the value for the "test" key) runs `vite build --mode=development
--outDir dist-tests` and writes output to dist-tests before running testem.
---
Nitpick comments:
In @.try.mjs:
- Around line 3-9: The embedded "ember-cli-build.cjs" template literal is not
linted or parsed, so either replace the inline string with reading a real
fixture file (use fs.readFileSync to load the on-disk ember-cli-build.cjs
content and use that string instead of the template literal) or add a minimal
smoke parse at module load that attempts to parse the template with new
Function(<template>) (or similar) and throws/logs on SyntaxError; update the
code paths that reference the template literal (the ember-cli-build.cjs value in
this file) to use the loaded/validated string so syntax regressions fail early.
- Around line 19-124: The scenarios block repeats nearly identical compat
scenarios (ember-lts-3.28 → ember-lts-5.12) differing only by ember-source and a
couple flags; factor this into a small helper to DRY it. Add a function (e.g.,
compatScenario(name, emberSourceVersion, opts = {})) near the top that returns
the scenario object using existing symbols compatDeps and compatFiles, accept
options for skipDeclarations and extraPins (to inject the
ember-cli/ember-page-title pins for 'ember-lts-3.28'), then replace the seven
inline objects in the scenarios array with calls to
compatScenario('ember-lts-3.28','~3.28.0',{skipDeclarations:true,
extraPins:{'ember-cli':'^4.12.0','ember-page-title':'~8.2.4'}}) and similar
calls for 4.4/4.8/4.12/5.4/5.8/5.12 (use skipDeclarations:true for 3.28–4.12),
preserving env keys like ENABLE_COMPAT_BUILD and SKIP_DECLARATIONS when
requested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6533f741-4ce5-40c8-8676-2e69be8753f0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.try.mjspackage.jsonrollup.config.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- rollup.config.mjs
| "lint:publish": "pnpm build && publint run --level error", | ||
| "prepack": "rollup --config && cp src/index.d.ts declarations/index.d.ts", | ||
| "start": "vite dev", | ||
| "test": "vite build --mode=development --out-dir dist-tests && testem --file testem.cjs ci --port 0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vite 7.x vite buildCLI accept--out-diras an alias for--outDir, or is only --outDir supported?
💡 Result:
No, Vite 7.x vite build CLI does not accept --out-dir as an alias for --outDir. Only --outDir is supported. The official Vite 7 documentation lists --outDir (camelCase) as the CLI option for specifying the output directory, with no mention of --out-dir (kebab-case) as an alias. This is consistent across Vite 7 docs at v7.vite.dev/guide/cli and the current vite.dev/guide/cli (which aligns with Vite 7+). The Vite source code in packages/vite/src/node/cli.ts explicitly defines the build command option as .option('--outDir
', '[string] output directory (default: dist)'), confirming --outDir is the registered flag. No kebab-case alias --out-dir is defined. Changelogs for Vite 7.x show no changes introducing --out-dir support. Config uses camelCase build.outDir, and CLI follows this convention for most options (e.g., --assetsDir, not --assets-dir).Citations:
- 1: https://v7.vite.dev/guide/cli
- 2: https://vite.dev/guide/cli
- 3: https://github.com/vitejs/vite/blob/v7.3.1/packages/vite/CHANGELOG.md
- 4: https://github.com/vitejs/vite/blob/v7.3.2/packages/vite/CHANGELOG.md
- 5: https://main.vite.dev/guide/cli
- 6: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/cli.ts
Change --out-dir to --outDir in the test script.
Vite 7.x only supports --outDir (camelCase) for the build command; the kebab-case --out-dir is not accepted and will be silently ignored. This causes the build output to default to dist/ instead of the intended dist-tests/, making tests run against the wrong files.
Current test script
"test": "vite build --mode=development --out-dir dist-tests && testem --file testem.cjs ci --port 0"
Update to:
"test": "vite build --mode=development --outDir dist-tests && testem --file testem.cjs ci --port 0"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 49, Update the "test" npm script in package.json to use
Vite's camelCase flag: change the Vite build option from --out-dir to --outDir
so the "test" script (the value for the "test" key) runs `vite build
--mode=development --outDir dist-tests` and writes output to dist-tests before
running testem.
|
😀 |
|
@esbanarango would it be possible to get this merged and released? |
3 participants
This PR migrates
ember-model-validatorto a V2 addon matching the latest structure https://github.com/ember-cli/ember-addon-blueprintThere are no functional changes (or shouldn't be).
Summary by CodeRabbit