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

chore: improve @magicbell/react-headless cjs/esm support #391

Merged
merged 9 commits into from
Oct 1, 2024

Conversation

smeijer
Copy link
Member

@smeijer smeijer commented Sep 27, 2024

fixes cjs/esm support by building with tshy. Needed to upgrade immer from v9 to v10 to support the case.

Copy link

vercel bot commented Sep 27, 2024

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

Name Status Preview Comments Updated (UTC)
playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2024 1:24pm

Copy link

pkg-pr-new bot commented Sep 27, 2024

@magicbell/cli

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/embeddable

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/core

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/in-app

yarn add https://pkg.pr.new/@magicbell/[email protected]

magicbell

yarn add https://pkg.pr.new/[email protected]

@magicbell/project-client

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/magicbell-react

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/react-headless

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/user-client

yarn add https://pkg.pr.new/@magicbell/[email protected]

@magicbell/webpush

yarn add https://pkg.pr.new/@magicbell/[email protected]

commit: 83e481e

Copy link
Member

@stigi stigi left a comment

Choose a reason for hiding this comment

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

Thanks @smeijer for this PR, I'll close #390 in favor of this one.

I've left a few comments inline, mostly questions and tiny suggestions though. The point I feel the strongest about is hardcoding __PACKAGE_NAME__ when we could import package.json as well.

packages/react-headless/src/index.ts Show resolved Hide resolved
packages/react-headless/src/lib/realtime.ts Show resolved Hide resolved
import { IRemoteNotification } from '../types';
import INotificationRepository from './INotificationRepository';
import INotificationStore from './INotificationStore';
import { IRemoteNotification } from '../types/index.js';
Copy link
Member

Choose a reason for hiding this comment

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

💅 I think it's better to directly import here, instead of going through index.ts, as we're in the same package.

Suggested change
import { IRemoteNotification } from '../types/index.js';
import IRemoteNotification from './IRemoteNotification.js';

Copy link
Member Author

@smeijer smeijer Sep 30, 2024

Choose a reason for hiding this comment

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

Agree, we should drop all barrel files. They're bad for bundlers / tree shaking. I didn't do it here, as I simply added the extensions using https://www.npmjs.com/package/fix-esm-import-path.

packages/react-headless/package.json Show resolved Hide resolved
@smeijer
Copy link
Member Author

smeijer commented Sep 30, 2024

Agreed about dropping the "magic constants". I didn't like the package.json imports, because not all bundlers might support importing from json files, but I came up with something better by using https://npmjs.com/replace-in-file right after the build script.

Now, we have a single place where we can import the constants from lib/pkg.ts, which makes it native JS and typed, and we have a build step that replaces the constants in dist/{esm|commonjs}/lib/pkg.js with the proper values.

@stigi
Copy link
Member

stigi commented Oct 1, 2024

@smeijer let me know if you need additional support here, like testing, delegating tasks or pairing, but it's mostly done as I can see.

@smeijer smeijer merged commit 60d24da into main Oct 1, 2024
8 checks passed
@smeijer smeijer deleted the feature/headless-tshy branch October 1, 2024 14:20
@MagicBella MagicBella mentioned this pull request Oct 1, 2024
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