-
Notifications
You must be signed in to change notification settings - Fork 68
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: remove commonjs bundle split points #417
Conversation
@@ -96,7 +96,7 @@ | |||
}, | |||
{ | |||
"path": "./packages/react/dist/browser/runtime.js", | |||
"limit": "489B", | |||
"limit": "445B", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice ❤️
color: blue; | ||
border: 1px solid black; | ||
`; | ||
// It's notoriously hard to do both server and client rendering in this test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
*/ | ||
const StyleCacheProvider: ProviderComponent = (props) => { | ||
if (isNodeEnvironment()) { | ||
// This code path isn't conditionally called at build time - safe to ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 🤔 It will work we know. Just wondering won't it confuse developers who are so familiar with rules of hooks? Is there a way we can split it into two components like we did before and do something like:
const StyleCacheProvider = isNodeEnvironment() ? StyleServerCacheProvider: StyleClientCacheProvider;
or maybe
const StyleCacheProvider: ProviderComponent = (props) => {
if (isNodeEnvironment()) {
return <StyleServerCacheProvider {...props} />
}
return <StyleClientCacheProvider {...props} />
}
Its still difficult to do as above for useCache
though 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this keeps the bundlesize down and keeps the react tree as small as possible - i wouldn't want to make more TBH
pros & cons - in this instance having good amounts of comments should work - i'll update them a bit though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries..hehe 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This PR merges the two browser/main bundles into one to remove the
require
call in the code - enabling strict mode modules to function correctly.Interestingly this also reduces the bundle size slightly.
Closes #405