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: remove commonjs bundle split points #417

Merged
merged 4 commits into from
Dec 20, 2020
Merged

Conversation

itsdouges
Copy link
Collaborator

@itsdouges itsdouges commented Dec 19, 2020

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

@itsdouges itsdouges requested a review from pgmanutd December 19, 2020 02:06
@@ -96,7 +96,7 @@
},
{
"path": "./packages/react/dist/browser/runtime.js",
"limit": "489B",
"limit": "445B",
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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 😞

Copy link
Collaborator Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries..hehe 😃

Copy link
Contributor

@pgmanutd pgmanutd left a comment

Choose a reason for hiding this comment

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

lgtm

@itsdouges itsdouges merged commit 4ddcf11 into master Dec 20, 2020
@itsdouges itsdouges deleted the merge-provider branch December 20, 2020 22:38
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.

Commonjs bundle split points don't work in strict bundlers
2 participants