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

Use static exports conditions to select node and browser code #156

Merged
merged 1 commit into from
Jul 1, 2023

Conversation

Andarist
Copy link
Contributor

⚠️ it is not exactly meant to be merged - at least not right now.

This PR is really just me tinkering with settings and stuff to get a better feeling about what's possible and how this can look like. Since I already tinkered with it locally, I figured out I can share it publicly here as well.

In this library though... this is really on the verge of micro-optimization though because we literally shave just a couple of bytes in production mode. I wonder though if there is something to be gained by wrapping all effects with isBrowser check. That could remove quite a bit of redundant code in node: less code parsing, less execution while SSRing, profit (?).

Feel free to close it right away.

@vercel
Copy link

vercel bot commented Jun 10, 2023

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

Name Status Preview Comments Updated (UTC)
react-resizable-panels ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 10, 2023 8:58am

@@ -0,0 +1 @@
export const isBrowser = typeof window !== "undefined";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This kinda should almost never be even chosen but better to be safe than sorry, right? :p

"#is-browser": {
"browser": "./src/env-conditions/browser.ts",
"node": "./src/env-conditions/node.ts",
"default": "./src/env-conditions/unknown.ts"
Copy link
Owner

Choose a reason for hiding this comment

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

I like most of this stuff! and I appreciate you trying things out and sharing ideas too!

My main concern here is that this is getting pretty large and easy to mess up. One conditional is 🤷🏼 but two is a lot worse 😁

"exports": {
".": {
"types": {
"import": "./dist/react-resizable-panels.cjs.mjs",
"default": "./dist/react-resizable-panels.cjs.js"
},
"development": {
"browser": {
"module": "./dist/react-resizable-panels.browser.development.esm.js",
"import": "./dist/react-resizable-panels.browser.development.cjs.mjs",
"default": "./dist/react-resizable-panels.browser.development.cjs.js"
},
"node": {
"module": "./dist/react-resizable-panels.development.node.esm.js",
"import": "./dist/react-resizable-panels.development.node.cjs.mjs",
"default": "./dist/react-resizable-panels.development.node.cjs.js"
},
"module": "./dist/react-resizable-panels.development.esm.js",
"import": "./dist/react-resizable-panels.development.cjs.mjs",
"default": "./dist/react-resizable-panels.development.cjs.js"
},
"browser": {
"module": "./dist/react-resizable-panels.browser.esm.js",
"import": "./dist/react-resizable-panels.browser.cjs.mjs",
"default": "./dist/react-resizable-panels.browser.cjs.js"
},
"node": {
"module": "./dist/react-resizable-panels.node.esm.js",
"import": "./dist/react-resizable-panels.node.cjs.mjs",
"default": "./dist/react-resizable-panels.node.cjs.js"
},
"module": "./dist/react-resizable-panels.esm.js",
"import": "./dist/react-resizable-panels.cjs.mjs",
"default": "./dist/react-resizable-panels.cjs.js"
},
"./package.json": "./package.json"
},

Copy link
Owner

Choose a reason for hiding this comment

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

I wish there was a more compact way to define/manage this.

const useIsomorphicLayoutEffect = canUseEffectHooks
? useLayoutEffect
: () => {};
const useIsomorphicLayoutEffect = isBrowser ? useLayoutEffect : () => {};
Copy link
Owner

Choose a reason for hiding this comment

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

This is nice.

@bvaughn
Copy link
Owner

bvaughn commented Jun 10, 2023

I'm on the fence about this. It's nice overall, except for the package.json explosion. 😄 What do you think?

@Andarist
Copy link
Contributor Author

I think that this kinda boils down to what you, as a maintainer, are comfortable with. I certainly see how the exports explosion is somewhat off-putting and I don't intend to try to sell it as a holy grail for everything. Even when it comes to the browser/SSR paths there are some edge cases because tools might use browser when bundling for workers (webworkers, edge workers, etc). So to do things the right way your have to also "override" browser-specific files with ones that are more oriented for SSRing in worker environments (often nothing special has to be done in them except of choosing the server-like paths). You can see how we do this in Emotion here - the end result is what you are not fond of though a bigger exports manifest.

To quote Sebastian's thoughts from this comment:

The principle that underlines this is static optimization of various combinations. In the ecosystem we shouldn’t have to boot code that includes all possible runtime branches. We will also move to use export conditions for production/development for example to support ESM for similar reasons.

This goes both ways that the client shouldn’t have to download server specific things for the branches that uses server-only things but the server also shouldn’t have to boot a bunch of code that isn’t relevant on the server.

So that’s why it’s worth having two separate implementations/entry points to the api that can each be optimal.

Yes, it’s a burden to library authors to optimize code but it’s a one time thing that helps the end user at the end of the day.

@bvaughn
Copy link
Owner

bvaughn commented Jun 23, 2023

I'm still struggling a bit with how to use these new conditions (e.g. bvaughn/suspense#35) so I'm a little wary to add more. I'll leave this open for the time being and keep thinking on it.

@Andarist
Copy link
Contributor Author

I'm still struggling a bit with how to use these new conditions (e.g. bvaughn/suspense#35)

i’ll try to take a look at this over the weekend

@bvaughn
Copy link
Owner

bvaughn commented Jun 23, 2023

As always I would appreciate your input, but please, only do that if you're personally curious.

@bvaughn
Copy link
Owner

bvaughn commented Jul 1, 2023

I've let this sit in the back of my mind for a while, and while I don't like the bloat it adds to package.json, I do support the general direction this is going in, so let's do it.

@bvaughn bvaughn force-pushed the static-platform-conditions branch from f303d95 to 4404e42 Compare July 1, 2023 14:25
@bvaughn bvaughn merged commit e0c082a into bvaughn:main Jul 1, 2023
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