Revert "import service instead of inject from '@ember/service'"#418
Revert "import service instead of inject from '@ember/service'"#418
Conversation
📝 WalkthroughWalkthroughThis PR systematically refactors service injection patterns across the codebase by removing compatibility fallback logic that selected between Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
🧹 Nitpick comments (1)
package.json (1)
25-25: Volta Node 24.14.1 pin is repo-wide and will enforce Node 24 for test-app-3.x and contributors using Volta.The root volta configuration will be inherited by
test-app-3.x(which extends../package.json), forcing it to Node 24.14.1. All test apps declare compatible engines (>= 18), but there is no documented Node version requirement in the repo. Verify this Node 24 requirement is intentional and compatible with your Ember/test-app testing strategy before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 25, The root package.json currently pins Volta's Node version via the "node": "24.14.1" entry which will be inherited by test-app-3.x and all contributors using Volta; decide whether you intended to force Node 24 across the repo—if not, either remove or relax this pin (e.g., remove the "node" entry or change it to a range compatible with your test apps like ">=18") or explicitly document and justify the Node 24 requirement in the repo README and update test-app-3.x engines if needed; locate the "node" key in package.json to make the change and ensure any workspace/test-app engine fields remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 25: The root package.json currently pins Volta's Node version via the
"node": "24.14.1" entry which will be inherited by test-app-3.x and all
contributors using Volta; decide whether you intended to force Node 24 across
the repo—if not, either remove or relax this pin (e.g., remove the "node" entry
or change it to a range compatible with your test apps like ">=18") or
explicitly document and justify the Node 24 requirement in the repo README and
update test-app-3.x engines if needed; locate the "node" key in package.json to
make the change and ensure any workspace/test-app engine fields remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a85a29e7-8eb0-42da-beb7-54f9440aba69
📒 Files selected for processing (11)
ember-moment/src/helpers/-base.jspackage.jsontest-app-3.x/app/controllers/index.jstest-app-3.x/app/routes/application.jstest-app-3.x/app/routes/index.jstest-app-4.x/app/controllers/index.jstest-app-4.x/app/routes/application.jstest-app-4.x/app/routes/index.jstest-app-5.x/app/controllers/index.jstest-app-5.x/app/routes/application.jstest-app-5.x/app/routes/index.js
NullVoxPopuli
left a comment
There was a problem hiding this comment.
Approving due to conversation in private about how the change here is technically only supported in 3.28 here, and since ember-moment 10 supports 3.16, we need to actually move this change to an ember-moment 11
Reverts #415
This was a real breaking change 🙈 I'm going to follow up with another PR that un-reverts it and drops support for unsupported Ember versions 👍
Edit: I also bumped node since we need require(esm) and the node version doesn't matter for the v2 addon 👍