-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add MUI exports to re-exports list #3095
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3095 +/- ##
==========================================
- Coverage 61.07% 60.92% -0.15%
==========================================
Files 597 597
Lines 27377 27440 +63
Branches 6624 6625 +1
==========================================
- Hits 16720 16718 -2
- Misses 10323 10388 +65
Partials 334 334
|
I was actually not aware of the purpose of the "list" module as being used for this, so probably indeed didn't have that right Can we make list.ts be a programmatic export of Object.keys(modules) so it doesn't need to be kept in sync with modules.ts to simplify it? e.g. list.ts
|
packages/core/ReExports/modules.tsx
Outdated
@@ -230,6 +232,8 @@ const libs = { | |||
useGridApiContext, | |||
useGridApiRef, | |||
useGridRootProps, | |||
GridToolbar, |
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.
could possibly make these into lazy imports. since it is a named import, you can synthesize a default export with an extra file, similar things done in e.g. https://github.com/GMOD/jbrowse-components/blob/main/packages/core/ReExports/Attributes.tsx
The list is usually imported in the rollup config file for a plugin, and making it a simple list of strings avoids having to load all the shared modules when building a plugin. We could modify the check at the end of the modules to make sure the list matches the libs keys exactly. There was some reason I don't fully remember that they weren't enforced to be the same, but I thing it's changed enough that they can be the same now. |
should be fine if it remains a list of strings if it is better for the plugin system. might help to have comment at top of list.ts explaining the relationship or something like this |
I added a check to make sure the lists are in sync, and a comment in the I also added exports of lazy versions of all the components in the DataGrid so that they can be used by plugins. |
})), | ||
), | ||
GridCheckCircleIcon: lazy(() => | ||
import('@mui/x-data-grid').then(module => ({ |
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 is probably ok, but note that this would put everything from x-data-grid in one bundle. splitting into separate might get slightly more efficient splitting but the main goal is kicking x-data-grid into its own bundle, so this isn't too bad https://reactjs.org/docs/code-splitting.html#named-exports
When upgrading to
@jbrowse/core
to v2.0.1 in Apollo, I found out that MUI was breaking the build because the re-exports list didn't include the MUI packages. This list is what tells plugins which packages are included globally by JBrowse and thus don't need to be packaged into the plugin.I generated an updated list from the
libs
inpackages/core/ReExports/modules.tsx
, basically usingThis also includes a couple more exports in
@mui/x-data-grid
that I want to use in Apollo.