Skip to content
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.

Sign up for GitHub

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

Fix SignalValue component when using react-transform #422

Closed
wants to merge 21 commits into from
Closed

Fix SignalValue component when using react-transform #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6cbfb73
=== BEGIN fix-signal-text === Add transform test for signal as text
andrewiggins Oct 5, 2023
3970b11
Fix SignalValue component when using react-transform
andrewiggins Sep 29, 2023
75a34c9
Skip test that now fails
andrewiggins Oct 5, 2023
9bd31c8
Add additional test for running nested try/finallys
andrewiggins Oct 11, 2023
1d6ab07
Add some sample test cases to think about in regards to nested useSig…
andrewiggins Oct 11, 2023
6b388bf
Add notes about states useSignals calls may transition between
andrewiggins Oct 11, 2023
3e9d63e
Add more useSignals tests to go and fix
andrewiggins Oct 13, 2023
8630054
Add test ids for generated tests
andrewiggins Oct 13, 2023
885f005
Add test for using signals with components that use render props
andrewiggins Oct 14, 2023
bd3fe1a
Add test for out-of-order effect error in React 16
andrewiggins Oct 17, 2023
694c605
Simplify test case a bit
andrewiggins Oct 17, 2023
758d093
Update test again to present passing state
andrewiggins Oct 17, 2023
9a7652c
Add notes about possible fix for out-or-order effect
andrewiggins Oct 17, 2023
a00a589
Encapsulate set/clearCurrentStore into store instance methods
andrewiggins Oct 17, 2023
fe812ee
Implement first pass of handling unmanaged and managed transitions
andrewiggins Oct 18, 2023
d7e75ee
Simplify start logic using methods
andrewiggins Oct 18, 2023
3b8f7a6
Update comments
andrewiggins Oct 18, 2023
0908221
Temporarily skip test to fix later
andrewiggins Oct 18, 2023
3268bc6
Update transform to emit usage enum to useSignals
andrewiggins Oct 19, 2023
40cf143
Remove test id stuff
andrewiggins Oct 19, 2023
b45b041
Update SignalValue and auto code to pass in useSignals(usage)
andrewiggins Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/react-transform/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ function isValueMemberExpression(
);
}

const tryCatchTemplate = template.statements`var STORE_IDENTIFIER = HOOK_IDENTIFIER();
const tryCatchTemplate = template.statements`var STORE_IDENTIFIER = HOOK_IDENTIFIER(HOOK_USAGE);
try {
BODY
} finally {
Expand All @@ -212,6 +212,11 @@ function wrapInTryFinally(
tryCatchTemplate({
STORE_IDENTIFIER: stopTrackingIdentifier,
HOOK_IDENTIFIER: get(state, getHookIdentifier)(),
HOOK_USAGE: isCustomHook(path)
? "2"
: isComponentFunction(path)
? "1"
: "",
BODY: t.isBlockStatement(path.node.body)
? path.node.body.body // TODO: Is it okay to elide the block statement here?
: t.returnStatement(path.node.body),
Expand Down
58 changes: 58 additions & 0 deletions packages/react-transform/test/browser/e2e.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,22 @@ describe("React Signals babel transfrom - browser E2E tests", () => {
checkHangingAct();
});

it("should rerender components when using signals as text", async () => {
const { App } = await createComponent(`
export function App({ name }) {
return <div>Hello {name}</div>;
}`);

const name = signal("John");
await render(<App name={name} />);
expect(scratch.innerHTML).to.equal("<div>Hello John</div>");

await act(() => {
name.value = "Jane";
});
expect(scratch.innerHTML).to.equal("<div>Hello Jane</div>");
});

it("should rerender components when signals they use change", async () => {
const { App } = await createComponent(`
export function App({ name }) {
Expand Down Expand Up @@ -194,6 +210,48 @@ describe("React Signals babel transfrom - browser E2E tests", () => {
expect(scratch.innerHTML).to.equal("<div>Hello John!</div>");
});

it.skip("should work when an ambiguous function is manually transformed and used as a hook", async () => {
// TODO: Warn/Error when manually opting in ambiguous function into transform
// TODO: Update transform to skip try/finally when transforming ambiguous function
const { App, greeting, name } = await createComponent(`
import { signal } from "@preact/signals-core";

export const greeting = signal("Hello");
export const name = signal("John");

// Ambiguous if this function is gonna be a hook or component
/** @trackSignals */
function usename() {
return name.value;
}

export function App() {
const name = usename();
return <div>{greeting.value} {name}</div>;
}`);

await render(<App name={name} />);
expect(scratch.innerHTML).to.equal("<div>Hello John</div>");

await act(() => {
greeting.value = "Hi";
});
expect(scratch.innerHTML).to.equal("<div>Hi John</div>");

await act(() => {
name.value = "Jane";
});
expect(scratch.innerHTML).to.equal("<div>Hi Jane</div>");

await act(() => {
batch(() => {
greeting.value = "Hello";
name.value = "John";
});
});
expect(scratch.innerHTML).to.equal("<div>Hello John</div>");
});

it("loads useSignals from a custom source", async () => {
const { App } = await createComponent(
`
Expand Down
32 changes: 16 additions & 16 deletions packages/react-transform/test/node/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand All @@ -90,7 +90,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>{name.value}</div>;
} finally {
Expand All @@ -113,7 +113,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand All @@ -137,7 +137,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = function () {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand Down Expand Up @@ -234,7 +234,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -256,7 +256,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = /** @trackSignals */() => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -280,7 +280,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -304,7 +304,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
export default function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand Down Expand Up @@ -352,7 +352,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
export function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -376,7 +376,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
export const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -400,7 +400,7 @@ describe("React Signals Babel Transform", () => {
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
/** @trackSignals */
export const MyComponent = function () {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand Down Expand Up @@ -871,7 +871,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -893,7 +893,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
return <div>Hello World</div>;
} finally {
Expand All @@ -916,7 +916,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
function MyComponent() {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand All @@ -940,7 +940,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "@preact/signals-react/runtime";
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand Down Expand Up @@ -1074,7 +1074,7 @@ describe("React Signals Babel Transform", () => {
const expectedOutput = `
import { useSignals as _useSignals } from "custom-source";
const MyComponent = () => {
var _effect = _useSignals();
var _effect = _useSignals(1);
try {
signal.value;
return <div>Hello World</div>;
Expand Down
43 changes: 30 additions & 13 deletions packages/react/runtime/src/auto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,14 @@ function installCurrentDispatcherHook() {
isEnteringComponentRender(currentDispatcherType, nextDispatcherType)
) {
lock = true;
store = useSignals();
store = useSignals(1);
lock = false;
} else if (
isRestartingComponentRender(currentDispatcherType, nextDispatcherType)
) {
store?.f();
lock = true;
store = useSignals(1);
lock = false;
} else if (
isExitingComponentRender(currentDispatcherType, nextDispatcherType)
Expand Down Expand Up @@ -281,18 +288,6 @@ function isEnteringComponentRender(
// are used to warn when hooks are nested improperly and do not indicate
// entering a new component render.
return false;
} else if (nextDispatcherType & RerenderDispatcherType) {
// Any transition into the rerender dispatcher is the beginning of a
// component render, so we should invoke our hooks. Details below.
//
// ## In-place rerendering (e.g. Mount -> Rerender)
//
// If we are transitioning from the mount, update, or rerender dispatcher to
// the rerender dispatcher (e.g. HooksDispatcherOnMount to
// HooksDispatcherOnRerender), then this component is rerendering due to
// calling setState inside of its function body. We are re-entering a
// component's render method and so we should re-invoke our hooks.
return true;
} else {
// ## Resuming suspended mount edge case (Update -> Mount)
//
Expand All @@ -316,6 +311,28 @@ function isEnteringComponentRender(
}
}

function isRestartingComponentRender(
currentDispatcherType: DispatcherType,
nextDispatcherType: DispatcherType
): boolean {
// A transition from a valid browser dispatcher into the rerender dispatcher
// is the restart of a component render, so we should end the current
// component effect and re-invoke our hooks. Details below.
//
// ## In-place rerendering (e.g. Mount -> Rerender)
//
// If we are transitioning from the mount, update, or rerender dispatcher to
// the rerender dispatcher (e.g. HooksDispatcherOnMount to
// HooksDispatcherOnRerender), then this component is rerendering due to
// calling setState inside of its function body. We are re-entering a
// component's render method and so we should re-invoke our hooks.

return Boolean(
currentDispatcherType & BrowserClientDispatcherType &&
nextDispatcherType & RerenderDispatcherType
);
}

/**
* We are exiting a component render if the current dispatcher is a valid
* dispatcher and the next dispatcher is the ContextOnlyDispatcher.
Expand Down
Loading
Loading