-
Notifications
You must be signed in to change notification settings - Fork 60
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 extra re-exports for default modules #2288
Conversation
Why is circularchordrenderer different? Would a breaking change be possible to sync it up? |
Notably that renderer class is an empty stub so could also be...removed? |
I'm not sure why it's different. I think it might be good to add this as a non-breaking change that allows a consistent way of accessing the renderers for now, and then think about doing a breaking change later. |
As discussed in a previous meeting, this now adds these re-exports so they are only a single level deep in the package, i.e. |
Codecov Report
@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
- Coverage 61.17% 61.09% -0.08%
==========================================
Files 538 538
Lines 25079 25079
Branches 5857 5857
==========================================
- Hits 15341 15322 -19
- Misses 9420 9439 +19
Partials 318 318
Continue to review full report at Codecov.
|
* Add extra re-exports for default modules * Make re-exports not so deep in the package * Fix name
There are some inconsistencies in the way things are re-exported from core. For example, most of the pluggable element types look something like this:
But the render type looks like this:
Most of the renderers are like that, but the chord renderer is different:
jbrowse-components/packages/core/ReExports/modules.ts
Lines 63 to 67 in 67a19b3
This has some implications for how these are used at runtime. To properly get the TrackType at runtime, you have to do:
But for RendererType you have to add a
.default
The real problem gets to be when a build system is trying to compile a plugin. These two imports looks similar:
but have to get compiled differently to use the global JBrowseExports properly. Our current UMD plugin build does some things to try to smooth this out, which work most of the time, but building to other targets doesn't work as well. Also, if core were ever used directly as a module running in Node, you'd have to worry about putting
.default
s in the right places as well.The change in this PR doesn't change any of this behavior, since I didn't want to break anything, but it does add some exports for the renderers so there is a consistent and named (not default) way of importing them that build systems can understand more easily.