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

Add ClientOnly #1592

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add ClientOnly #1592

wants to merge 2 commits into from

Conversation

lxsmnsyc
Copy link
Member

@lxsmnsyc lxsmnsyc commented Mar 3, 2023

Summary

Finally took the initiative. This one is the missing piece to SSR components, as it has some common use cases (e.g. rendering a component with web-only dependency). solid-start has one but I'm just hoping that core is the best place to put this so that other SSR frameworks (like Astro) can also use this. The fact that this is one of the most requested solution in solid-start and possibly other SSR applications should be enough to motivate this to be in the core.

How did you test this change?

The code is pretty basic, based on my 2-year old snippet: https://discord.com/channels/722131463138705510/722167424186843267/1022433627000291348

I just added some hydration checks + server code should be optimized to just return undefined directly

@changeset-bot
Copy link

changeset-bot bot commented Mar 3, 2023

⚠️ No Changeset found

Latest commit: 9d92908

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

export function ClientOnly(props: {
children?: JSX.Element,
}): JSX.Element {
if (sharedConfig.context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

iirc this is the way to check if the app is currently hydrating. This check is needed so that UI correction can be applied, otherwise we can just render the children directly (since the app is just in CSR)

@coveralls
Copy link

coveralls commented Mar 3, 2023

Pull Request Test Coverage Report for Build 4323318523

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 9 (0.0%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 87.808%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/solid/src/render/flow.ts 0 9 0.0%
Files with Coverage Reduction New Missed Lines %
packages/solid/src/render/component.ts 13 70.11%
Totals Coverage Status
Change from base Build 4260487064: -0.4%
Covered Lines: 1299
Relevant Lines: 1405

💛 - Coveralls

@@ -258,3 +259,23 @@ export function ErrorBoundary(props: {
"_SOLID_DEV_" ? { name: "value" } : undefined
) as Accessor<JSX.Element>;
}

export function ClientOnly(props: {
children?: JSX.Element,
Copy link

@emdede emdede Mar 3, 2023

Choose a reason for hiding this comment

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

Is there a case where you'd want to use a self-closing <ClientOnly />?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe 🤷 🤣

Choose a reason for hiding this comment

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

If there is a play, there is a player 😆

@ryansolid
Copy link
Member

ryansolid commented Mar 5, 2023

My only concern with this and why I did this with dynamic imports/compiler macro in SolidStart is if this will be sufficient for bundlers to drop the code. We told people to do this and they were having issues still since they used libraries that touched global APIs (like window, document) so this alone wasn't enough. At which point I was less into this approach on its own.

I'm still trying to figure out if we can work this into Bling. But it is very framework specific when accounting for hydration.

@lxsmnsyc
Copy link
Member Author

lxsmnsyc commented Mar 5, 2023

yeah I could probably do a lazy-like HOC. This wrapping component still has its place IMO

@thetarnav
Copy link
Contributor

Combining <ClientOnly> with lazy should be enough to drop the code, right? As a component, it can be easily composed.

const MyLazyComponent = lazy(() => import("..."))

<ClientOnly>
  <Suspense>
    <MyLazyComponent />
  </Suspense>
</ClientOnly>

@lxsmnsyc
Copy link
Member Author

@thetarnav yes it should be. I provided clientOnly just in case it wasn't enough and as stated by Ryan, users might mistake on using ClientOnly first before the lazy version

@RodrigoProjects
Copy link

Is this still not merged for some reason? Just for context! :)

@ryansolid
Copy link
Member

We have it in SolidStart so there hasn't been pressure to bring it back into core yet. Still trying it out so to speak.

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.

6 participants