start emitting declarations during the build - extracted from the JS files#417
start emitting declarations during the build - extracted from the JS files#417NullVoxPopuli merged 1 commit intomasterfrom
Conversation
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ember-moment/tsconfig.json (1)
3-18: Make theunpublished-development-typescontract explicit.With
rootDirpinned to./src, TypeScript errors when an included non-declaration source file lives outside that tree. If this directory is intended to stay ambient-only, narrowing the glob to./unpublished-development-types/**/*.d.tswould encode that invariant and avoid a future TS6059 footgun. (typescriptlang.org)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ember-moment/tsconfig.json` around lines 3 - 18, The tsconfig.json currently includes "./unpublished-development-types/**/*" while rootDir is set to "./src", which causes TS6059 if non-declaration files live outside src; narrow the include to only declaration files by changing the include entry referencing "unpublished-development-types" to "./unpublished-development-types/**/*.d.ts" so that the compiler treats that directory as ambient-only and avoids conflicts with the "rootDir" setting (update the "include" array entry in tsconfig.json that references unpublished-development-types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ember-moment/package.json`:
- Around line 18-22: Add an explicit JS-only export entry for the "./_app_/*"
paths before the existing "./*" export in the package.json "exports" block so
TypeScript won't match "./_app_/*" into the wildcard and incorrectly advertise
declarations; specifically, carve out "./_app_/*" with only a runtime/default
export (no "types" entry) and place it above the "./*" mapping that currently
provides "types": "./declarations/*.d.ts" and "default": "./dist/*.js" to ensure
declarations from "declarations/" are not (falsely) applied to _app_ artifacts.
In `@ember-moment/rollup.config.mjs`:
- Around line 56-57: Update the TypeScript declaration-generation command used
by addon.declarations to include --emitDeclarationOnly so tsc only emits .d.ts
files and does not produce JS that conflicts with Rollup/Babel; locate the call
addon.declarations('declarations', 'tsc --declaration') and change the command
string to include --emitDeclarationOnly (e.g., 'tsc --declaration
--emitDeclarationOnly').
---
Nitpick comments:
In `@ember-moment/tsconfig.json`:
- Around line 3-18: The tsconfig.json currently includes
"./unpublished-development-types/**/*" while rootDir is set to "./src", which
causes TS6059 if non-declaration files live outside src; narrow the include to
only declaration files by changing the include entry referencing
"unpublished-development-types" to "./unpublished-development-types/**/*.d.ts"
so that the compiler treats that directory as ambient-only and avoids conflicts
with the "rootDir" setting (update the "include" array entry in tsconfig.json
that references unpublished-development-types).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 108e5774-6ae2-4c80-a1ac-c61116ee3d4c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
ember-moment/package.jsonember-moment/rollup.config.mjsember-moment/tsconfig.json
| "./addon-main.js": "./addon-main.cjs", | ||
| "./*": { | ||
| "types": "./declarations/*.d.ts", | ||
| "default": "./dist/*.js" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the package.json file
find . -name "package.json" -type f | head -20Repository: adopted-ember-addons/ember-moment
Length of output: 203
🏁 Script executed:
# Read the main package.json to inspect the export map
cat -n ./package.json | head -100Repository: adopted-ember-addons/ember-moment
Length of output: 1435
🏁 Script executed:
# Read the ember-moment/package.json file which is the one being reviewed
cat -n ./ember-moment/package.jsonRepository: adopted-ember-addons/ember-moment
Length of output: 4987
🏁 Script executed:
# Check the tsconfig.json to understand declaration generation
cat -n ./ember-moment/tsconfig.jsonRepository: adopted-ember-addons/ember-moment
Length of output: 897
🏁 Script executed:
# Check the build output and directory structure
ls -la ./ember-moment/dist/ 2>/dev/null || echo "dist/ doesn't exist yet"
ls -la ./ember-moment/declarations/ 2>/dev/null || echo "declarations/ doesn't exist yet"Repository: adopted-ember-addons/ember-moment
Length of output: 132
🏁 Script executed:
# Check the rollup configuration to understand the build output structure
cat -n ./ember-moment/rollup.config.mjs | head -80Repository: adopted-ember-addons/ember-moment
Length of output: 3043
🏁 Script executed:
# Let's check if there's a build output or if we can inspect the src directory structure
ls -la ./ember-moment/src/ | head -30Repository: adopted-ember-addons/ember-moment
Length of output: 415
🏁 Script executed:
# Let's check what files are in the src/helpers and src/services directories
find ./ember-moment/src -type f -name "*.js" -o -name "*.gjs" | head -20Repository: adopted-ember-addons/ember-moment
Length of output: 918
🌐 Web query:
TypeScript package.json exports longest prefix pattern matching module resolution
💡 Result:
TypeScript's module resolution for package.json "exports" field uses longest prefix pattern matching, analogous to its "paths" mapping resolution. In TypeScript's module resolution (under moduleResolution: "node16", "nodenext", or "bundler"), when resolving an import specifier against a package's "exports" map, it selects the export key with the longest matching prefix for the subpath. This matches Node.js's PACKAGE_EXPORTS_RESOLVE algorithm, which iterates over exports keys (in package.json object order) and uses the first (longest-matching) subpath pattern or exact match. For subpath patterns (keys ending in ""), matching replaces "" with the corresponding suffix from the requested subpath. TypeScript prioritizes "exports" over legacy fields like "typesVersions", "types", and "main" when present. To enable: Use "moduleResolution": "node16"/"nodenext"/"bundler" and "resolvePackageJsonExports": true (default in those modes). Example package.json: { "exports": { ".": "./index.js", "./foo": "./foo.js", "./bar/": "./bar/.js" } } Import "pkg/bar/baz" matches "./bar/*" (longest prefix "./bar/" > "."). This prevents access to unexported subpaths, enforcing encapsulation.
Citations:
- 1: https://www.typescriptlang.org/docs/handbook/modules/reference.html
- 2: https://nodejs.org/api/packages.html
- 3: https://nodejs.org/api/modules.html
- 4: Resolution Algorithm Specification fixing nodejs/node#49402
- 5: https://stackoverflow.com/questions/70296652/how-can-i-use-exports-in-package-json-for-nested-submodules-and-typescript
- 6: https://www.typescriptlang.org/tsconfig/resolvePackageJsonExports.html
Add explicit ./_app_/* export to prevent false type advertisements.
The wildcard ./* export pattern matches _app_/* paths (from ember-addon.app-js), but TypeScript's longest-prefix matching means if ./_app_/* is defined, it takes precedence. Currently, the ./* pattern advertises declarations/_app_/**.d.ts types that won't be generated—the build only creates declarations from src/ (via tsconfig.json's rootDir: "./src"), not from the _app_/ build artifact. Add an explicit JS-only ./_app_/* export before ./* to carve those paths out cleanly and prevent false type resolution.
📦 Suggested change
"exports": {
".": {
"types": "./declarations/index.d.ts",
"default": "./dist/index.js"
},
"./addon-main.js": "./addon-main.cjs",
+ "./_app_/*": "./dist/_app_/*.js",
"./*": {
"types": "./declarations/*.d.ts",
"default": "./dist/*.js"
}
},📝 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.
| "./addon-main.js": "./addon-main.cjs", | |
| "./*": { | |
| "types": "./declarations/*.d.ts", | |
| "default": "./dist/*.js" | |
| } | |
| "./addon-main.js": "./addon-main.cjs", | |
| "./_app_/*": "./dist/_app_/*.js", | |
| "./*": { | |
| "types": "./declarations/*.d.ts", | |
| "default": "./dist/*.js" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ember-moment/package.json` around lines 18 - 22, Add an explicit JS-only
export entry for the "./_app_/*" paths before the existing "./*" export in the
package.json "exports" block so TypeScript won't match "./_app_/*" into the
wildcard and incorrectly advertise declarations; specifically, carve out
"./_app_/*" with only a runtime/default export (no "types" entry) and place it
above the "./*" mapping that currently provides "types": "./declarations/*.d.ts"
and "default": "./dist/*.js" to ensure declarations from "declarations/" are not
(falsely) applied to _app_ artifacts.
| // Emit .d.ts declaration files | ||
| addon.declarations('declarations', 'tsc --declaration'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine it
find . -name "rollup.config.mjs" -type fRepository: adopted-ember-addons/ember-moment
Length of output: 109
🏁 Script executed:
# Read the rollup.config.mjs file to understand context
cat -n ember-moment/rollup.config.mjs 2>/dev/null || find . -name "rollup.config.mjs" -exec cat -n {} +Repository: adopted-ember-addons/ember-moment
Length of output: 3043
🏁 Script executed:
# Look for package.json to understand dependencies
find . -name "package.json" -type f | head -5Repository: adopted-ember-addons/ember-moment
Length of output: 203
🏁 Script executed:
# Check for tsconfig files
find . -name "tsconfig.json" -type fRepository: adopted-ember-addons/ember-moment
Length of output: 105
🏁 Script executed:
# Read the main tsconfig.json
cat -n tsconfig.json 2>/dev/null || cat -n ./ember-moment/tsconfig.json 2>/dev/null || find . -name "tsconfig.json" -exec cat -n {} +Repository: adopted-ember-addons/ember-moment
Length of output: 897
🌐 Web query:
TypeScript emitDeclarationOnly --declaration allowJs documentation
💡 Result:
The TypeScript compiler options --emitDeclarationOnly, --declaration, and allowJs work together to generate declaration files (.d.ts) from JavaScript files without emitting JavaScript output. Key points: - emitDeclarationOnly: true (or --emitDeclarationOnly): Instructs the compiler to only emit .d.ts files, skipping .js files. Useful when using another transpiler for JS or providing types for consumers. - declaration: true (or --declaration): Enables generation of .d.ts files. Required when using emitDeclarationOnly. - allowJs: true (or --allowJs): Allows the compiler to process .js files as part of the program, enabling type checking and declaration generation from them. To generate .d.ts files from .js files: 1. Create a tsconfig.json: { "compilerOptions": { "allowJs": true, "declaration": true, "emitDeclarationOnly": true, "outDir": "dist" // Optional: directs output to a separate directory }, "include": ["src//.js"] } 2. Run tsc (uses tsconfig.json) or CLI: tsc src//.js --allowJs --declaration --emitDeclarationOnly --outDir dist. This setup processes .js files (thanks to allowJs), generates corresponding .d.ts files (via declaration + emitDeclarationOnly), and avoids overwriting original .js files (use outDir). Examples from docs: - CLI: tsc index.js --declaration --emitDeclarationOnly - For JS projects incrementally adopting TS. Note: Cannot combine emitDeclarationOnly with noEmit. For jsconfig.json projects, there may be compatibility issues with --emitDeclarationOnly. Official docs confirm this combination for creating .d.ts from .js files, especially with JSDoc for better types.
Citations:
- 1: https://www.typescriptlang.org/tsconfig/emitDeclarationOnly.html
- 2: https://www.typescriptlang.org/docs/handbook/compiler-options.html
- 3: https://www.typescriptlang.org/tsconfig/allowJs.html
- 4: https://typescriptlang.org/tsconfig/declaration.html
- 5: https://typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html
Add --emitDeclarationOnly to the declaration generation command.
This build uses Rollup/Babel to emit JavaScript. When TypeScript generates declarations from JavaScript with allowJs enabled, it should run in declaration-only mode to avoid emitting JavaScript that would conflict with Rollup's output. TypeScript's documentation explicitly recommends pairing --declaration with --emitDeclarationOnly in this scenario.
Suggested change
- addon.declarations('declarations', 'tsc --declaration'),
+ addon.declarations('declarations', 'tsc --declaration --emitDeclarationOnly'),📝 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.
| // Emit .d.ts declaration files | |
| addon.declarations('declarations', 'tsc --declaration'), | |
| // Emit .d.ts declaration files | |
| addon.declarations('declarations', 'tsc --declaration --emitDeclarationOnly'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ember-moment/rollup.config.mjs` around lines 56 - 57, Update the TypeScript
declaration-generation command used by addon.declarations to include
--emitDeclarationOnly so tsc only emits .d.ts files and does not produce JS that
conflicts with Rollup/Babel; locate the call addon.declarations('declarations',
'tsc --declaration') and change the command string to include
--emitDeclarationOnly (e.g., 'tsc --declaration --emitDeclarationOnly').
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTypeScript support is being added to the ember-moment addon through updated package exports with type definitions, new TypeScript configuration, modified build process to generate declarations, updated development dependencies, and upgraded Node.js version. An import pattern in the test application is simplified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 25: You changed the Node engine to "24.14.1" in package.json (the "node"
engines entry); before committing, verify all dependencies and CI/tools support
Node 24, run full test suites and linting under Node 24, update any
incompatibility (bump or pin offending packages) or revert to a compatible range
(e.g., maintain >=18 until ready), and if this is intentional add a migration
note to CONTRIBUTING.md or the PR describing required local setup and any
breaking changes to help contributors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36ea9fb7-1cd4-4c20-8949-ed2dbba31ee4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
ember-moment/package.jsonember-moment/rollup.config.mjsember-moment/tsconfig.jsonpackage.jsontest-app-3.x/app/controllers/index.js
| }, | ||
| "volta": { | ||
| "node": "18.20.3", | ||
| "node": "24.14.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js 24 release date LTS
💡 Result:
Node.js 24 was released on May 6, 2025, as the initial Current release (v24.0.0). It entered Active LTS status on October 28, 2025, with v24.11.0. As of March 24, 2026, it is in Active LTS (codename Krypton), with Active LTS until October 20, 2026, Maintenance LTS until April 30, 2028, and full EOL on April 30, 2028.
Citations:
- 1: https://nodejs.org/blog/release/v24.0.0
- 2: https://versionlog.com/nodejs/24/
- 3: https://nodejs.org/en/blog/release/v24.0.0
- 4: https://nodejs.org/en/about/previous-releases
- 5: https://endoflife.date/nodejs
- 6: https://nodejs.org/en/blog/release/v24.14.0
- 7: https://nodejs.org/blog/release/v24.14.0
- 8: https://nodejs.org/en/blog/release/v24.11.0
- 9: Upcoming Node.js Major Release (v24) nodejs/node#57057
- 10: https://github.com/nodejs/Release/blob/main/schedule.json
- 11: https://github.com/nodejs/Release?hss_channel=tw-91985735
- 12: 2025-05-06, Version 24.0.0 (Current) nodejs/node#57609
Ensure Node.js 24 compatibility across dependencies before major version upgrade.
Node.js 24 is now in Active LTS (since October 2025) and 24.14.1 is a valid, supported version. However, this represents a jump of 6 major versions from Node 18. Before committing to this upgrade, verify that:
- All project dependencies support Node 24 without breaking changes
- Contributors' local environments can reasonably support Node 24
- Breaking changes between Node 18 and 24 don't impact the codebase
If this change is intentional for a major version bump, add a clear migration guide or update CONTRIBUTING.md accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 25, You changed the Node engine to "24.14.1" in
package.json (the "node" engines entry); before committing, verify all
dependencies and CI/tools support Node 24, run full test suites and linting
under Node 24, update any incompatibility (bump or pin offending packages) or
revert to a compatible range (e.g., maintain >=18 until ready), and if this is
intentional add a migration note to CONTRIBUTING.md or the PR describing
required local setup and any breaking changes to help contributors.
✅ Actions performedReviews paused. |
mansona
changed the title
2 participants
We don't need to convert this project to TS to get it to start emitting some useful types 🎉
Here is an example of what it is generating straight from the JS with no JSDoc
@typecomments anywhere:Let me know if you have any questions or objections 👍
Edit: I bumped the node version used in CI (from the volta definition in pacakge.json) because of require(esm) problems and it's not relevant to the deployed v2 addon 👍Edit: I fixed CI last night with other PRs and releases 👍 this should be ready to review+merge