-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fix React adapter in SSR and when rerendering #355
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
7f34c3b
Add reproduction of sync component rerenders causing failures
andrewiggins 828e701
Temporarily skip the renderToStaticMarkup test for now. Will address …
andrewiggins 68d292b
Update test to call unmount to do actual React unmount
andrewiggins 77d7298
Update component entering check to work for react 18 prod bundle
andrewiggins 6cd9ccf
Update logic to explicitly discover rerender dispatcher
andrewiggins 2261447
Ignore timeout errors logged to console
andrewiggins 1a4e5e6
Explicitly skip over transitions to and from development warning disp…
andrewiggins 028667d
Attempt to fixing spuriously failing test involving cleanup effects b…
andrewiggins 32bdd27
Add state machine as a comment and a TODO to update the old comment
andrewiggins b101fa2
Reenable static markup test
andrewiggins f3df910
Move current react tests to browser folder
andrewiggins 83ec845
Add initial renderToStaticMarkup test with basic test infra
andrewiggins d3ea8e5
Simplify NodeJS babel config since Node tests always run against buil…
andrewiggins 83e5a06
Add some more comments and investigations to look at
andrewiggins 6d0651f
Properly detect server render dispatchers and do nothing
andrewiggins 693b84d
Add basic SSR tests and share with browser tests
andrewiggins 30b54ee
Improve rerender dispatcher detection
andrewiggins 9e6a4e0
Ignore other useless console errors
andrewiggins 8a8c0d3
Add more tests for rerendering
andrewiggins 792ad2e
Run server tests on minify
andrewiggins 1e2f9f3
Add test for state-less hooked component
andrewiggins 7c1e4a6
Update implementation comments
andrewiggins 5a37285
Add test for changing signal during mount
andrewiggins 492bddd
Update comment on state machine
andrewiggins 21c8ee9
Add changeset
andrewiggins eba0b93
Update CI to build then test
andrewiggins 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 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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@preact/signals-react": patch | ||
--- | ||
|
||
Fix React adapter in SSR and when rerendering |
This file contains 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
This file contains 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
Large diffs are not rendered by default.
Oops, something went wrong.
File renamed without changes.
This file contains 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 |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { mountSignalsTests } from "../shared/mounting"; | ||
import { | ||
Root, | ||
createRoot, | ||
act, | ||
getConsoleErrorSpy, | ||
checkConsoleErrorLogs, | ||
} from "../shared/utils"; | ||
|
||
describe("@preact/signals-react mounting", () => { | ||
let scratch: HTMLDivElement; | ||
let root: Root; | ||
|
||
async function render(element: JSX.Element | null): Promise<string> { | ||
await act(() => root.render(element)); | ||
return scratch.innerHTML; | ||
} | ||
|
||
beforeEach(async () => { | ||
scratch = document.createElement("div"); | ||
document.body.appendChild(scratch); | ||
root = await createRoot(scratch); | ||
getConsoleErrorSpy().resetHistory(); | ||
}); | ||
|
||
afterEach(async () => { | ||
scratch.remove(); | ||
checkConsoleErrorLogs(); | ||
}); | ||
|
||
mountSignalsTests(render); | ||
}); |
This file contains 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
This file contains 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
This file contains 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 |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { signal, useSignalEffect } from "@preact/signals-react"; | ||
import { expect } from "chai"; | ||
import { createElement } from "react"; | ||
import { renderToStaticMarkup } from "react-dom/server"; | ||
import sinon from "sinon"; | ||
import { mountSignalsTests } from "../shared/mounting"; | ||
|
||
describe("renderToStaticMarkup", () => { | ||
mountSignalsTests(renderToStaticMarkup); | ||
|
||
it("should not invoke useSignalEffect", async () => { | ||
const spy = sinon.spy(); | ||
const sig = signal("foo"); | ||
|
||
function App() { | ||
useSignalEffect(() => spy(sig.value)); | ||
return <p>{sig.value}</p>; | ||
} | ||
|
||
const html = await renderToStaticMarkup(<App />); | ||
expect(html).to.equal("<p>foo</p>"); | ||
expect(spy.called).to.be.false; | ||
}); | ||
}); |
This file contains 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 |
---|---|---|
@@ -0,0 +1,52 @@ | ||
// import register from "@babel/register"; | ||
const register = require("@babel/register").default; | ||
|
||
const coverage = String(process.env.COVERAGE) === "true"; | ||
|
||
// @babel/register doesn't hook into the experimental NodeJS ESM loader API so | ||
// we need all test files to run as CommonJS modules in Node | ||
const env = [ | ||
"@babel/preset-env", | ||
{ | ||
targets: { | ||
node: "current", | ||
}, | ||
loose: true, | ||
modules: "commonjs", | ||
}, | ||
]; | ||
|
||
const jsx = [ | ||
"@babel/preset-react", | ||
{ | ||
runtime: "classic", | ||
pragma: "createElement", | ||
pragmaFrag: "Fragment", | ||
}, | ||
]; | ||
|
||
const ts = [ | ||
"@babel/preset-typescript", | ||
{ | ||
jsxPragma: "createElement", | ||
jsxPragmaFrag: "Fragment", | ||
}, | ||
]; | ||
|
||
register({ | ||
extensions: [".js", ".mjs", ".ts", ".tsx", ".mts", ".mtsx"], | ||
cache: true, | ||
|
||
sourceMaps: "inline", | ||
presets: [ts, jsx, env], | ||
plugins: [ | ||
coverage && [ | ||
"istanbul", | ||
{ | ||
// TODO: Currently NodeJS tests always run against dist files. Should we | ||
// change this? | ||
// include: minify ? "**/dist/**/*.js" : "**/src/**/*.{js,jsx,ts,tsx}", | ||
}, | ||
], | ||
].filter(Boolean), | ||
}); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: coverage isn't reported from this test run and I got tired and didn't set up running tests against source. If someone is feeling motivated and wants to do so, please do!