-
Notifications
You must be signed in to change notification settings - Fork 232
feature/RHTL-68 – server-side-rendering #510
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
Conversation
This isn't ready but i've made the move over into 'native' and thought it'd be good to track along side #498 as that is also a huge change to the codebase.
the .eslintrc
config is not being picked up hence the lint errors. Also I can't actually run validate
or lint
because of the following error
Error: .eslintrc » ./node_modules/kcd-scripts/eslint.js » /Users/joshua/code/react-hooks-testing-library/node_modules/kcd-scripts/node_modules/eslint-config-kentcdodds/jest.js:
Environment key "jest/globals" is unknown
My assumption this is related to #502 any ideas @marcosvega91?
Hi @joshuaellis have you reinstalled all modules ? Are you using yarn
or npm
?.
If you are using yarn
remember to remove your local yarn.lock
then Delete node_modules
folder and install all libraries again. Let me know if this solve your problem
Hi @marcosvega91,
If you are using yarn remember to remove your local yarn.lock then Delete node_modules folder and install all libraries again.
I was using yarn
. But doing the above method gave the same error. repeating steps above and re-installing with npm
worked fine. Does kcd-scripts
not support yarn? Weird if not.
You're right. I'm receiving the same error. I'll try to understand why with yarn is giving this problem. Thanks for raising it 😄
The problem with yarn is that is not resolving correctly eslint
. yarn
is using 6.8.0
from gatsby
will it should use the version 7.15.0
from kcd-scripts
Hi @mpeyper,
I've just migrated the previous server work you did & the server/suspenseHook
fails because -
ReactDOMServer does not yet support Suspense. 31 | renderProps = props 32 | baseAct(() => { > 33 | const serverOutput = ReactDOMServer.renderToString(toRender(props)) | ^ 34 | container.innerHTML = serverOutput 35 | }) 36 | },
I just wanted to know if you remember this happening to you or not? IMO i can't initially see why this test would fail as we're not using suspense
. Maybe i'm not understanding the error well enough. If you're not sure dw.
@joshuaellis Yes, this was an issue for me too.
@joshuaellis the TS migration (#498, #520) is in a beta release now. I suggest we rebase these changes on the [beta
branch(https://github.com/testing-library/react-hooks-testing-library/tree/beta) before continuing.
Great stuff @mpeyper thanks for letting me know.
I've managed to navigate alot of the Typescript stuff so far, I'm really struggling how to solve the issue in native/pure
with the toRender
–
const toRender = (props?: TProps): JSX.Element => ( <Wrapper {...props}> <TestHook hookProps={props} {...testHookProps} /> </Wrapper> )
the issue i'm getting is
Type '{ children: Element; }' is not assignable to type 'TProps'.
'TProps' could be instantiated with an arbitrary type which could be unrelated to '{ children: Element; }'.
I was wondering if it had something to do with this on going issue – DefinitelyTyped/DefinitelyTyped#34237 if anyone has ideas, they're definitely welcome.
I've managed to navigate alot of the Typescript stuff so far, I'm really struggling how to solve the issue in
native/pure
with thetoRender
–const toRender = (props?: TProps): JSX.Element => ( <Wrapper {...props}> <TestHook hookProps={props} {...testHookProps} /> </Wrapper> )the issue i'm getting is
Type '{ children: Element; }' is not assignable to type 'TProps'.
'TProps' could be instantiated with an arbitrary type which could be unrelated to '{ children: Element; }'.I was wondering if it had something to do with this on going issue – DefinitelyTyped/DefinitelyTyped#34237 if anyone has ideas, they're definitely welcome.
DM, it actually had to do with this issue microsoft/TypeScript#28938 (comment)
I updated the base branch to reduce the noise in the review and I think we'll want this to honour as a beta first anyway.
I've had a brief look on my phone just now and I like where this is headed. I have a few comments to leave, but I'll wait until I'm on my laptop to do so.
Stil to do this comment & define some tests for ‘src/pure’
Stil to do this comment & define some tests for ‘src/pure’
I've fixed the types for the submodules now and I've removed the custom
module (but left the generics as discussed in that comment).
I worked around the type issue by generating the submodules instead of having static files for them. In some ways I like this solution more, but I'm still a bit upset I couldn't figure out the TS configuration to get it to work.
As for tests of the root module, I'm still not sure how to do this as we have both dependencies installed so testing the error cases or even getting it to select the dom
renderer is going to be a challenge. I'm not a fan of exporting functions just for testing, but it might be the only way for this one? I'm also happy to put it on the back burner for now. The logic is fairly straight forward and unlikely to change much in the future.
Other than that, I think it's just the docs that need some love on this one now.
I think we should release this into a beta now that the previous beta has been released. What do you think @joshuaellis?
Absolutely, let’s push to beta. I can begin work on the docs later today 💪🏼
🎉 This PR is included in version 5.0.0-beta.1 🎉
The release is available on:
Your semantic-release bot 📦🚀
hmmmm... I was expecting that to publish as 5.0.0-beta.1
. The bot even picked up the BREAKING CHANGES
in the release tag?
bad bot. That is weird, I would have expected that too.
to trigger the release of the 5
beta I guess you should merge master into beta first and then merge the PR into the beta branch.
Your master branch has a commit Merge branch 'beta'
that is linked to the 4
version.
Anyway BREAKING CHANGE
is read by semantic release and considered as a major release
Thanks @marcosvega91, I have rebuilt the beta
branch no with the 4.0.0
tag in the history so the next build should be right 🤞
I just tried the beta out in one of my own projects. All renderers are working great and the auto-detection is also working. The only issue so far is that when niether react-dom
or react-test-renderer
are installed, the error message is coming out as
Error: Could not auto-detect a React renderer. Are you sure you've installed one of the following - react-dom/n - react-test-renderer
Whoops. I'll fix that quickly and get a new build out.
Uh oh!
There was an error while loading. Please reload this page.
What:
New feature SSR testing – breaking change.
Split the repo into
dom|native|server
Why:
#68
How:
RTR does not currently support SSR,
react-dom
does, this will be the default renderer for SSR testing. However, the plan is to expose acustom
renderer usage where users can use other renderers e.gpreact
. User's will have their test engine autoChecklist: