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

Add missing named exports to shared core modules #2051

Merged
merged 3 commits into from
Jun 18, 2021
Merged

Conversation

garrettjstevens
Copy link
Collaborator

For a few core files in out re-exports, we were only re-exporting the default export for those file and not the named exports. This adds re-exports of the named exports as well.

@garrettjstevens garrettjstevens self-assigned this Jun 14, 2021
@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jun 14, 2021
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #2051 (8b6b6c5) into main (96e4efe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2051   +/-   ##
=======================================
  Coverage   60.84%   60.84%           
=======================================
  Files         473      473           
  Lines       22569    22569           
  Branches     5252     5252           
=======================================
  Hits        13733    13733           
  Misses       8554     8554           
  Partials      282      282           
Impacted Files Coverage Δ
packages/core/ReExports/modules.ts 75.00% <ø> (ø)
...pluggableElementTypes/renderers/BoxRendererType.ts 74.35% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96e4efe...8b6b6c5. Read the comment docs.

@tchannagiri
Copy link

tchannagiri commented Jun 15, 2021

Hi @garrettjstevens, I don't mean to add more to your plate but do you think 'RendererType'.ts and 'FeatureRendererType.ts' could be re-exported as well? FeatureRendererType is in the class-hierarchy between BoxRendererType and ServerSideRendererType so its a bit strange to leave it out, and RendererType is the base class of them all. These two might not have any issues compiling, but it does lead to code duplication in the UMD file, which makes it difficult to debug because then it's hard to tell which file to set the breakpoint in.

Also, in BoxRendererType.ts it might be nice to have these also re-exported since LayoutSession depends on these interfaces.

interface LayoutSessionProps {
  config: AnyConfigurationModel
  bpPerPx: number
  filters: SerializableFilterChain
}

type MyMultiLayout = MultiLayout<GranularRectLayout<unknown>, unknown>
interface CachedLayout {
  layout: MyMultiLayout
  config: AnyConfigurationModel
  filters: SerializableFilterChain
}

I've been noodling around with these files quite a bit, do you think it would be helpful for me to submit a PR and have you review it? If it helps I can show you my use case and how I extended BoxRendererType/LayoutSession.

Edit: Maybe this is a silly question, but why isn't just every file in '@jbrowse/core' fully re-exported? I can't think of really any downside to this: there doesn't seem to be overhead to doing this, and it will reduce the code duplication in UMD plugins builds.

Thanks,
TJ

@rbuels rbuels added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jun 15, 2021
@garrettjstevens
Copy link
Collaborator Author

@tchannagiri no problem at all, it's good to hear what people are using and want to use. Let me know if that last commit addressed all of the issues.

Theres's no real reason everything isn't exported, it just that @jbrowse/core has kind of grown organically for what we've needed and so we're still finding things that need to be optimized for e.g. external plugins. We've been thinking we might need to split our current core into multiple packages, though, so we can try to make sure we're more methodical in our exports when we do that.

@rbuels
Copy link
Contributor

rbuels commented Jun 16, 2021

Don't the modules.ts and list.ts both have to be updated together? The list (used to) be used by the plugin webpack build system to know which modules to mark as global and not in the bundle.

If list.ts is no longer used, should just delete it

@garrettjstevens
Copy link
Collaborator Author

You're right, they should have been added to the list as well. I've added them now. The list is used by the plugin build system.

@tchannagiri
Copy link

Thanks @garrettjstevens. Actually if you don’t mind could core/util/offscreenCanvasPonyfill.js and core/util/offscreenCanvasUtils.tsx be re-exported as well (I think core/util/index.ts might need to be modified for this)? Currently, I’m not using these directly (though they might be imported indirectly by another file), but re-exporting them might resolve this issue: GMOD/jbrowse-plugin-template#15, since I was getting compilation issues attempting to import them. I’m currently using a hack in tsdx.config.js to skip compiling the problematic import ("require(‘canvas’)" in offscreenCanvasPonyfill.js).

Otherwise, the current re-exports seems to resolve my issues with BoxRendererType and LayoutSession. I haven't been able to test this out yet due to having to work on other parts of the webside I'm working on, but once I'm able to I'll let you know if anything else comes up.

@rbuels rbuels merged commit 01de19b into main Jun 18, 2021
@rbuels rbuels deleted the more_exports_modules branch June 18, 2021 16:27
@garrettjstevens
Copy link
Collaborator Author

@tchannagiri I've got another PR in to fix GMOD/jbrowse-plugin-template#15 here: #2064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants