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

Feat: Add Individual Component Imports #267

Closed
wants to merge 5 commits into from

Conversation

tkottke90
Copy link
Contributor

Description

Related to #35. I started importing Kobalte into my Solid project and noticed that there was a significant (8-10 second based on a Google Lighthouse score) delay before my page had loaded.

image

Looking at the developer tools (specifically the network tab in Firefox) I could see that there was about 3.54 MB of files loaded into the browser. Based on the file names I could see that every Kobalte module was being loaded regardless of how many were being used:

image

Naturally I was curious about bundle sizes as some were in the related issue. I was happy to see this package was tree-shakeable

image

This drew me to the conclusion that it was simply a developer experience issue. The cost of having to re-load the entirety of Kobalte on any non-cached reload meant seeing a white screen while all of the resources downloaded to the browser.

Solution

Seeing the open issue and the conservation that was going on it appeared to me that this could be resolved by adding sub-folder exports to the core module:

import Dialog from "../../kobalte/dialog";

export function App() {

  return (
    <Dialog.Root>
      ...
    </Dialog.Root>
  );
}

To achieve this a sub-folder export was added to the package.json file which allowed each of the component directories to be targeted directly:

// package.json
{
  ...,
  exports: {
    ...,
    "./*": {
      "types": "./dist/types/*/index.d.ts",
      "solid": "./dist/source/*/index.jsx"
    }
  }
}

One of the goals I had when working on this project was to ensure backwards compatibility. To this effort a default export was added to each component module could be a "drop-in replacement" for the current version of Kobalte. This way consumers of this library could transition when they were ready or the Kobalte team can decide on if/when deprecation of the previous pattern would occur.

@netlify
Copy link

netlify bot commented Sep 10, 2023

Deploy Preview for kobalte failed.

Name Link
🔨 Latest commit c08d32d
🔍 Latest deploy log https://app.netlify.com/sites/kobalte/deploys/6578efe19d9dac0008559e5d

@Murzbul
Copy link

Murzbul commented Oct 25, 2023

What would be the problem with this feature? The issue of downloading and improving performance seems quite important to me.

@fabien-ml
Copy link
Collaborator

Hi, sorry for the late response. I've no problem with the PR, I just didn't had time to review it yet.

@seogrady
Copy link

@fabien-ml Any movement on this PR? It'd be great to get this in.

@tkottke90
Copy link
Contributor Author

Resolved the merge conflicts that were being reported so those don't block the merge

@@ -38,4 +38,6 @@ export type {
PaginationRootOptions,
PaginationRootProps,
};
export { Ellipsis, Item, Items, Next, Previous, Root };

export { Ellipsis, Item, List, Next, Previous, Root };
Copy link
Collaborator

Choose a reason for hiding this comment

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

PaginationList aka List is now PaginationItems aka Items

@thomas-evans
Copy link

@tkottke90 @fabien-ml Bumping: chrome_MiXbenOyZk
Thank you all for the work on this, if I can help please message me.

@@ -27,3 +27,4 @@ export type {
AccordionTriggerProps,
};
export { Content, Header, Item, Root, Trigger };
export default { Content, Header, Item, Root, Trigger };
Copy link
Contributor

@aminya aminya Apr 29, 2024

Choose a reason for hiding this comment

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

Exposing default exports is not necessary for this PR. The package.json change should be enough.
I made a simpler change needed for this capability in #390

@jer3m01
Copy link
Member

jer3m01 commented Apr 30, 2024

Integrated this in #391 and changed the API.
Thanks for your PR!

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.

7 participants