-
Notifications
You must be signed in to change notification settings - Fork 20
Add failing tests for #82 #95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Wendy Fouchter (Wendystraite)
wants to merge
1
commit into
saasquatch:main
Choose a base branch
from
Wendystraite:tests_issue_82
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1020,3 +1020,258 @@ strictModeSuite(({ wrapper, isStrict }) => { | |
| } | ||
| }); | ||
| }); | ||
|
|
||
| strictModeSuite(({ wrapper }) => { | ||
| describe("Issue #82 - Implicit and explicit scoped molecule instantiations", () => { | ||
| // NOTE: This issue was reported with React 18 strict mode behavior | ||
| // where components mount -> unmount -> remount, causing cache cleanup issues. | ||
| // React 19 changed strict mode behavior (useMemo is called twice but keeps first result) | ||
| // which has partially resolved this issue. These tests verify the expected behavior. | ||
|
|
||
| // Shared test setup for Issue #82 tests | ||
| const TestScope = createScope("test-id"); | ||
|
|
||
| function createTestMolecules() { | ||
| // Source molecule with unique random value | ||
| const sourceLifecycle = createLifecycleUtils(); | ||
| let sourceInstanceCount = 0; | ||
|
|
||
| const SourceMolecule = molecule(() => { | ||
| sourceInstanceCount++; | ||
| const value = { | ||
| count: Math.random(), | ||
| id: sourceInstanceCount, | ||
| value: `source-${sourceInstanceCount}`, | ||
| }; | ||
| sourceLifecycle.connect(value); | ||
| return value; | ||
| }); | ||
|
|
||
| // Other molecule that depends on Source | ||
| const otherLifecycle = createLifecycleUtils(); | ||
| const OtherMolecule = molecule(() => { | ||
| const source = use(SourceMolecule); | ||
| const value = { source, other: Math.random() }; | ||
| otherLifecycle.connect(value); | ||
| return value; | ||
| }); | ||
|
|
||
| return { | ||
| SourceMolecule, | ||
| OtherMolecule, | ||
| sourceLifecycle, | ||
| otherLifecycle, | ||
| getSourceInstanceCount: () => sourceInstanceCount, | ||
| }; | ||
| } | ||
|
|
||
| const StrictWrapper = wrapper; | ||
|
|
||
| function createTestWrapper() { | ||
| return ({ children }: { children?: React.ReactNode }) => ( | ||
| <StrictWrapper> | ||
| <ScopeProvider scope={TestScope} value="foo"> | ||
| {children} | ||
| </ScopeProvider> | ||
| </StrictWrapper> | ||
| ); | ||
| } | ||
|
|
||
| describe.each([ | ||
| { first: "explicit", second: "implicit" }, | ||
| { first: "implicit", second: "explicit" }, | ||
| ])("When molecule is first $first then $second", ({ first }) => { | ||
| test("Should share the same instance", () => { | ||
| const { | ||
| SourceMolecule, | ||
| OtherMolecule, | ||
| sourceLifecycle, | ||
| otherLifecycle, | ||
| } = createTestMolecules(); | ||
| const Wrapper = createTestWrapper(); | ||
|
|
||
| sourceLifecycle.expectUncalled(); | ||
| otherLifecycle.expectUncalled(); | ||
|
|
||
| let explicitSource: { count: number }; | ||
| let implicitSource: { count: number }; | ||
| let sourceCountAfterFirst: number; | ||
|
|
||
| if (first === "explicit") { | ||
| // Step 1: Mount Source molecule EXPLICITLY first | ||
| const { result: sourceResult } = renderHook( | ||
| () => useMolecule(SourceMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
|
|
||
| explicitSource = sourceResult.current; | ||
| sourceCountAfterFirst = sourceLifecycle.executions.mock.calls.length; | ||
|
|
||
| // Step 2: Mount Other molecule which implicitly uses Source | ||
| const { result: otherResult } = renderHook( | ||
| () => useMolecule(OtherMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
|
|
||
| implicitSource = otherResult.current.source; | ||
| } else { | ||
| // Step 1: Mount Other molecule (which implicitly creates Source) | ||
| const { result: otherResult } = renderHook( | ||
| () => useMolecule(OtherMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
|
|
||
| implicitSource = otherResult.current.source; | ||
| sourceCountAfterFirst = sourceLifecycle.executions.mock.calls.length; | ||
|
|
||
| // Step 2: Now explicitly mount Source molecule in the same scope | ||
| const { result: sourceResult } = renderHook( | ||
| () => useMolecule(SourceMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
|
|
||
| explicitSource = sourceResult.current; | ||
| } | ||
|
|
||
| // CRITICAL: The explicit and implicit instances should be THE SAME | ||
| expect(explicitSource).toBe(implicitSource); | ||
| expect(explicitSource.count).toBe(implicitSource.count); | ||
|
|
||
| // Source should NOT be executed again (already exists in cache) | ||
| expect(sourceLifecycle.executions.mock.calls.length).toBe( | ||
| sourceCountAfterFirst, | ||
| ); | ||
| }); | ||
|
|
||
| test("Should propagate changes between instances", () => { | ||
|
Wendystraite marked this conversation as resolved.
|
||
| const { SourceMolecule, OtherMolecule } = createTestMolecules(); | ||
| const Wrapper = createTestWrapper(); | ||
|
|
||
| let sourceValue: { count: number }; | ||
| let otherValue: { source: { count: number }; other: number }; | ||
|
|
||
| if (first === "explicit") { | ||
| // Mount Source first (explicit) | ||
| const { result: sourceResult } = renderHook( | ||
| () => useMolecule(SourceMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
| sourceValue = sourceResult.current; | ||
|
|
||
| // Mount Other second (implicit Source) | ||
| const { result: otherResult } = renderHook( | ||
| () => useMolecule(OtherMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
| otherValue = otherResult.current; | ||
| } else { | ||
| // Mount Other first (implicit Source) | ||
| const { result: otherResult } = renderHook( | ||
| () => useMolecule(OtherMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
| otherValue = otherResult.current; | ||
|
|
||
| // Mount Source second (explicit) | ||
| const { result: sourceResult } = renderHook( | ||
| () => useMolecule(SourceMolecule), | ||
| { wrapper: Wrapper }, | ||
| ); | ||
| sourceValue = sourceResult.current; | ||
| } | ||
|
|
||
| // They should be the same instance | ||
| expect(sourceValue).toBe(otherValue.source); | ||
|
|
||
| // Modify via explicit reference | ||
| sourceValue.count = 42; | ||
|
|
||
| // Changes should be visible in implicit reference | ||
| expect(otherValue.source.count).toBe(42); | ||
| }); | ||
|
|
||
| test("Should handle toggle pattern with real components", () => { | ||
|
Wendystraite marked this conversation as resolved.
|
||
| const { SourceMolecule, OtherMolecule, getSourceInstanceCount } = | ||
| createTestMolecules(); | ||
|
|
||
| // Component that explicitly uses Source | ||
| function ComponentA() { | ||
| const source = useMolecule(SourceMolecule); | ||
| return <div data-testid="component-a">{source.value}</div>; | ||
| } | ||
|
|
||
| // Component that uses Other (which implicitly uses Source) | ||
| function ComponentB() { | ||
| const other = useMolecule(OtherMolecule); | ||
| return <div data-testid="component-b">{other.source.value}</div>; | ||
| } | ||
|
|
||
| let initialValue: string; | ||
| let instancesAfterFirst: number; | ||
| let aValue: string; | ||
| let bValue: string; | ||
|
Comment on lines
+1209
to
+1212
|
||
|
|
||
| if (first === "explicit") { | ||
| // Main component with toggle - ComponentA (explicit) first | ||
| const TestComponent = ({ showB }: { showB: boolean }) => ( | ||
| <ScopeProvider scope={TestScope} value="test"> | ||
| <ComponentA /> | ||
| {showB && <ComponentB />} | ||
| </ScopeProvider> | ||
| ); | ||
|
|
||
| const { rerender, getByTestId } = render( | ||
| <TestComponent showB={false} />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // Initially, only ComponentA is mounted (explicit Source) | ||
| initialValue = getByTestId("component-a").textContent; | ||
| instancesAfterFirst = getSourceInstanceCount(); | ||
|
|
||
| // Now toggle on ComponentB (implicit Source via Other) | ||
| rerender(<TestComponent showB={true} />); | ||
|
|
||
| aValue = getByTestId("component-a").textContent; | ||
| bValue = getByTestId("component-b").textContent; | ||
| } else { | ||
| // Main component with toggle - ComponentB (implicit) first | ||
| const TestComponent = ({ showA }: { showA: boolean }) => ( | ||
| <ScopeProvider scope={TestScope} value="test"> | ||
| <ComponentB /> | ||
| {showA && <ComponentA />} | ||
| </ScopeProvider> | ||
| ); | ||
|
|
||
| const { rerender, getByTestId } = render( | ||
| <TestComponent showA={false} />, | ||
| { wrapper }, | ||
| ); | ||
|
|
||
| // Initially, only ComponentB is mounted (implicit Source) | ||
| initialValue = getByTestId("component-b").textContent; | ||
| instancesAfterFirst = getSourceInstanceCount(); | ||
|
|
||
| // Now toggle on ComponentA (explicit Source) | ||
| rerender(<TestComponent showA={true} />); | ||
|
|
||
| bValue = getByTestId("component-b").textContent; | ||
| aValue = getByTestId("component-a").textContent; | ||
| } | ||
|
|
||
| // CRITICAL TEST: Both components should show the SAME source value | ||
| // This is the bug from issue #82 - they were getting different instances | ||
| expect(aValue).toBe(bValue); | ||
| expect(aValue).toBe(initialValue); | ||
|
|
||
| // Source should NOT be created again when second component mounts | ||
| // (it should reuse the instance from first component) | ||
| expect(getSourceInstanceCount()).toBe(instancesAfterFirst); | ||
|
|
||
| // Both components should reference the same instance ID | ||
| expect(aValue).toContain("source-1"); | ||
| expect(bValue).toContain("source-1"); | ||
|
Wendystraite marked this conversation as resolved.
|
||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.