fix: devtools-syncer missing case for extension #226
Conversation
This makes `rxMutation` a fully autonomous function which can also be used entirely independently of the SignalStore. It also exposes the following additional properties: - `isSuccess` - `hasValue` - `value`
|
On Line 26 in I removed the unneeded |
|
I am not Michael, but I took a quick look. Would it be possible to construct a test case that causes the bug? |
Sounds good to remove for me
Good catch |
| }); | ||
|
|
||
| function createTrackerMock(): Tracker { | ||
| const onChangeMock = jest.fn(); |
There was a problem hiding this comment.
Didn't think about how this will translate to Vitest, but it should be fine.
There was a problem hiding this comment.
Something like this:
(tracker.onChange as ReturnType<typeof vi.fn>).mockImplementation((cb) => {
cb({ [id]: { count: 42 } });
});
| @@ -0,0 +1,96 @@ | |||
| import { PLATFORM_ID } from '@angular/core'; | |||
There was a problem hiding this comment.
Added a separate test file. I did look at other tests and noticed deprecated use of TestBed.flushEffects() should be able to be quickly replaced with TestBed.tick(). Best to fix during Vitest conversion PR or separate PR.
There was a problem hiding this comment.
I ran all of these tests against the current main branch.
All of them passed except "should handle extension absence gracefully".
That test failed only because it checks whether a warning message is shown when
extensions aren’t available.
Issue #213, however, shows an error from renameStore, which points to a
different problem than the one addressed in this PR.
We need a test that reproduces the reported error.
Other than that, I do think that you don't have to create a separate file.
Adding it to connecting.spec.ts should work.
rainerhahnekamp
force-pushed
the
main
branch
from
463ecfc to
efb3481
Compare
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Sorry to spoil the party, but there are a few things that need attention:
- The actual bug doesn’t seem to be fixed. I left a comment in the tests, but
instead of addressing that, you added extra warning messages (which had been
removed on purpose before). Also, there is still no test that reproduces the
reported issue. - I generally appreciate PRs that include refactorings. The problem here is
that by applying a different style, I also have to review those changes,
which shifts the focus away from the bug fix. If you want to do refactorings,
please open them in a separate PR.
Once we have a test that reproduces the bug (and fixes it), I think we’ll be on the right
track.
| connectSpy = jest.fn(() => ({ send: sendSpy })); | ||
| (window as any).__REDUX_DEVTOOLS_EXTENSION__ = { connect: connectSpy }; | ||
|
|
||
| TestBed.resetTestingModule(); |
There was a problem hiding this comment.
why do we need that?
| const name = this.#stores[id].name; | ||
| const name = this.#stores[id]?.name; | ||
|
|
||
| this.#stores = Object.entries(this.#stores).reduce( |
There was a problem hiding this comment.
We can discuss conventional vs. use-case-specific naming - both have their
pros and cons. In the end, it’s a matter of style.
For the sake of consistency, please revert to newStore here.
| newState[name] = state; | ||
| } | ||
| return newState; | ||
| (acc, [storeName, state]) => { |
| }, | ||
| {} as StoreRegistry, | ||
| ); | ||
| this.#stores = Object.entries(this.#stores).reduce((acc, [key, value]) => { |
| newState[storeName] = state; | ||
| } | ||
| return newState; | ||
| (acc, [storeName, state]) => { |
| } | ||
|
|
||
| getNextId() { | ||
| getNextId(): string { |
There was a problem hiding this comment.
Generally speaking, when you implement a fix or so, try
not to do too much refactoring. It makes the review process
longer...especially if we have a disagreement on how the
refactoring was done.
We can leave that, but we should have a general discussion if public
methods should explicitely define the return type.
|
|
||
| const dummyConnection: Connection = { | ||
| send: () => void true, | ||
| send: () => true, |
There was a problem hiding this comment.
why did you change the return type?
| #initDevtoolsConnection(): Connection { | ||
| const extension = window.__REDUX_DEVTOOLS_EXTENSION__; | ||
| if (!extension) { | ||
| console.warn('[DevtoolsSyncer] Redux DevTools extension not found.'); |
There was a problem hiding this comment.
i think it can be annoying to throw warnings if DevTools are not
there. Especially since they can also be enabled in production.
| constructor() { | ||
| if (!this.#isBrowser) { | ||
| return; | ||
| console.warn( |
There was a problem hiding this comment.
No warnings if we are on the server. We had
this before and users were complaining that
we are filling up their logs.
| @@ -0,0 +1,96 @@ | |||
| import { PLATFORM_ID } from '@angular/core'; | |||
There was a problem hiding this comment.
I ran all of these tests against the current main branch.
All of them passed except "should handle extension absence gracefully".
That test failed only because it checks whether a warning message is shown when
extensions aren’t available.
Issue #213, however, shows an error from renameStore, which points to a
different problem than the one addressed in this PR.
We need a test that reproduces the reported error.
Other than that, I do think that you don't have to create a separate file.
Adding it to connecting.spec.ts should work.
|
Closing in favor of #234. |
4 participants
Closes #213
With these changes, you no longer get a blank screen when using Redux DevTools extension.