Conversation
git-online-helper
bot
force-pushed
the
fix-ember-lts
branch
from
841491e to
813fb9d
Compare
patricklx
force-pushed
the
fix-ember-lts
branch
16 times, most recently
from
8c681aa to
b510ccb
Compare
BlueCutOfficial
left a comment
BlueCutOfficial
left a comment
There was a problem hiding this comment.
Hey @patricklx, I like how small and focused that PR is, it makes it easy to review and comment 👍 I posted a few questions / notes before approving.
| constructor(...args: unknown[]) { | ||
| // @ts-expect-error ignore | ||
| super(...args); |
There was a problem hiding this comment.
Why do you need this change?
There was a problem hiding this comment.
in 3.16 it fails with
Assertion Failed: An EmberObject based class, (unknown), was not instantiated correctly. You may have either used `new` instead of `.create()`, or not passed arguments to your call to super in the constructor: `super(...arguments)`. If you are trying to use `new`, consider using native classes without extending from EmberObject.
| teardownContext, | ||
| } from '@ember/test-helpers'; | ||
| import { run } from '@ember/runloop'; | ||
| import ERouter from '@ember/routing/route'; |
There was a problem hiding this comment.
What idea do you want to express with the name ERouter?
There was a problem hiding this comment.
Router was already used further down... and EmberRouter as well, any other good naming suggestion is welcome :D
There was a problem hiding this comment.
What you're importing is from @ember/routing/route though, to "reset" the application route and prevent the UI one from triggering side effects. So I would rather go with Route, I don't think this one is used already in the file.
| } | ||
| }); | ||
| }); | ||
| owner.register('service:port', class extends Service {}); |
There was a problem hiding this comment.
Do you know more precisely what part was causing the old tests to break?
There was a problem hiding this comment.
in the port service there is an import of internal @ember/-internals/metal, which didn't exist in older ember
|
|
||
| Application.initializer({ | ||
| name: `00-override-adapter`, | ||
| name: `setup-2-override-adapter`, |
There was a problem hiding this comment.
Same question about the name, what does it express?
There was a problem hiding this comment.
the name of the initialiser in init/setup is setup.
this ensures that this initialiser is sorted after ui inspector setup initializer
There was a problem hiding this comment.
Could we add a small in-code comment for that as well? 🙏
| this.owner.register('route:application', class extends ERouter {}); | ||
| this.owner.register('controller:application', class extends Controller {}); | ||
| this.owner.register('template:application', hbs('{{outlet}}')); |
There was a problem hiding this comment.
I feel that these lines deserve a comment in the code, to help understand how the tests are structured. Something similar to what the PR description says, but phrased for someone who starts looking at this with no context.
| /** | ||
| * preferably, ember debug tests should use their own test app | ||
| * but currently its mangled with the inspector ui app, which is not compatible with all ember versions being tested. | ||
| * we do filter the tests to only run the ember_debug tests, but that does not prevent the app merging. | ||
| * The application route/controller/template of inspector ui was being indirectly used in ember_debug tests, | ||
| * which is not required and broke older lts tests | ||
| */ |
3 participants
Description