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.

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

Server-side rendering with react #698

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

divdavem
Copy link
Member

@divdavem divdavem commented Apr 9, 2024

This PR is built on top of #697
It adds support for server-side rendering with react.

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 92.65%. Comparing base (bc879ba) to head (0f9949c).

Files Patch % Lines
react/headless/src/utils/directive.ts 86.95% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
- Coverage   92.72%   92.65%   -0.07%     
==========================================
  Files          69       69              
  Lines        1992     2002      +10     
  Branches      367      368       +1     
==========================================
+ Hits         1847     1855       +8     
- Misses         77       78       +1     
- Partials       68       69       +1     
Flag Coverage Δ
e2e 82.47% <87.50%> (-0.02%) ⬇️
unit 86.60% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@divdavem divdavem force-pushed the ssrReact branch 2 times, most recently from af224f7 to b104af0 Compare April 9, 2024 20:58
@divdavem divdavem marked this pull request as ready for review April 9, 2024 21:08
Copy link
Contributor

@quentinderoubaix quentinderoubaix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments

(directive: Directive): {ref: RefCallback<HTMLElement>};
<T>(directive: Directive<T>, args: T): {ref: RefCallback<HTMLElement>};
} = BROWSER
? <T>(directive: Directive<T>, args?: T): {ref: RefCallback<HTMLElement>; suppressHydrationWarning: true} => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep the hydration warning than suppress it to be fair...
We can merge with that warning, but please create a GitHub issue to track this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened #715 to track this.

core/src/components/accordion/accordion.ts Show resolved Hide resolved
react/bootstrap/src/components/accordion/accordion.tsx Outdated Show resolved Hide resolved
react/bootstrap/src/components/alert/alert.tsx Outdated Show resolved Hide resolved
react/headless/src/utils/directive.ts Show resolved Hide resolved
react/ssr-app/vite.config.ts Outdated Show resolved Hide resolved
@divdavem
Copy link
Member Author

divdavem commented Apr 19, 2024

@quentinderoubaix I have made the changes you requested. I am not at all satisfied with the reduced validation this implies (i.e. not running react SSR test in dev mode), but if it is ok for you, we can merge it like this and improve things later. I have added a comment in #315 about this.

Copy link
Contributor

@quentinderoubaix quentinderoubaix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@divdavem
Copy link
Member Author

@quentinderoubaix Thank you!

@divdavem divdavem merged commit 85a47ac into AmadeusITGroup:main Apr 19, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants