Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

fix(build): make build pass and fully esm bundle #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ecyrbe
Copy link

@ecyrbe ecyrbe commented May 4, 2023

  • export client and server correctly as node16 package
  • fix mising react-dom dep
  • pin and upgrade dependencies
  • put shared deps (react, zod) as peer dependencies
  • don't use use client on lib level, this is app responsibility

@vercel
Copy link

vercel bot commented May 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
zact-example ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 4, 2023 8:29pm

@@ -1,5 +1,3 @@
"use client";
Copy link
Member

Choose a reason for hiding this comment

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

Decent chance this won't work without this

Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty confident about this.
Here you are in a library.
Library code is built and bundled without knowing anything about RSC boundaries.
This should be at component level in your next app :

'use client'
import { useZact } from 'zact/client';

cf https://github.com/reactjs/rfcs/blob/main/text/0227-server-module-conventions.md

Copy link

@chungweileong94 chungweileong94 May 6, 2023

Choose a reason for hiding this comment

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

Well true, we should use this hook in client component anyway, user will have to specify the use client instead, so the directive is not really necessary in here.

However, it doesn't hurt to have the use client directive here tho, to tell React that this is one of the client entry, it's safer than nothing.

https://nextjs.org/docs/getting-started/react-essentials#third-party-packages

Copy link
Author

@ecyrbe ecyrbe May 6, 2023

Choose a reason for hiding this comment

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

The issue is, tsup used here to bundle does not honor 'use client' placement at the top of the file.
So that's why i propose to remove it, since it's not packaged correctly

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants