search

BimberLab / DiscvrLabKeyModules

A collection of public LabKey modules developed by the Bimber Lab
4 stars 4 forks source link

Follow up to gitter discussion on package import #205

Closed cmdcolin closed 6 months ago

cmdcolin commented 1 year ago

I just wanted to follow up discussion that was on gitter chat

The re-use of the VcfTabixAdapter via importing from the src directory may not necessarily be the intended form of re-use. that would be essentially importing the typescript src file directly, which is generally an odd NPM pattern in general (https://github.com/BimberLab/DiscvrLabKeyModules/blob/6a412c9354bb6abad69ab7cec16a533f47cd3a16/jbrowse/src/client/JBrowse/Browser/plugins/ExtendedVariantPlugin/ExtendedVariantAdapter/ExtendedVariantAdapter.ts#L7)

We don't actually have an 'intended' package re-export that can be re-imported currently (that would probably be an export from 'plugins/variants/src/index.ts' in our codebase)....We could consider adding one though

Another option is to use the 'getSubAdapter' concept. This is used in a number of places in our codebases. The SNPCoverageAdapter uses a CramAdapter as a subAdapter, the CramAdapter uses the sequenceAdapter as a subadapter, etc. Your extended variant adapter could have a VcfTabixAdapter subadapter.

There may be other options, but thought I'd just open the dialog. Can close issue if this isn't the appropriate forum though

cmdcolin commented 1 year ago

also note: it appears you are doing this extension to work around some performance issue. if you have any sample data or instance to reproduce, we could investigate too!

bbimber commented 1 year ago

Hey - thanks for following up. My use case is a couple of things:

I'm looking at this now, but I think the subadapter idea might be a solution for our existing import/extend in configSchema?

bbimber commented 1 year ago

With respect to SetMaxHeightDlg and isUTR, we are effectively grabbing private methods from JBrowse, and I understand how that's not a good pattern.

cmdcolin commented 1 year ago

there is actually a proposal in https://github.com/GMOD/jbrowse-components/pull/3771 to remove the "src" directory from our published artefacts. Another group was running into issues where the existence of the src directory was causing them typescript build errors. You could potentially just swap "src" with "dist" or "esm" depending on your use case (dist is commonjs, esm is "esm" style import/export code similar to the src directory). It would still be poking into sort of "private" type things that way, but just wanted to give you a heads up

bbimber commented 1 year ago

Thanks. It is on my radar to deal with some of the technical debt and fix how we do this. I have not 100% wrapped my head about subadapters (nor mobx, frankly). I will try to find some time to look more closely into this.

Beyond that, it's possible we would seek to export some methods currently private in JBrowse. After we look at this I will be about opening PRs in jbrowse, if that makes sense.

cmdcolin commented 1 year ago

sure thing :) can check out PRs if it comes up.

just for random add-ons:

the subadapters can basically let you "wrap" around another adapter using something sort of like "composition" instead of "inheritance" of the adapter perhaps. with subadapters you don't necessarily need to explicitly even know the type of thing you are wrapping around since it is dynamically loaded at runtime. whether that is of interest to your case is up to you.

the mobx stuff can be tricky indeed also. this was my attempt at a quick intro if it is any help. https://gist.github.com/cmdcolin/94d1cbc285e6319cc3af4b9a8556f03f

bbimber commented 11 months ago

@cmdcolin: Thanks. I did a refactor last week that upgraded us to the latest Jbrowse and dealt with most of the direct imports. I have one issue remaining around VcfTabixAdapter. Our previous code made a class that directly extended VcfTabixAdapter. The purpose was: 1) do add caching (this was per-related when viewing a very variant-dense region), and 2) Instead of VariantFeature we have an ExtendedVariantFeature class that calculates some custom fields.

In order to get this done, as a half-measure I copied VcfTabixAdapter into our project; however, I want to fix this. I understand your point around "subclassing" by composition; however, I might be missing something about the examples from BAM/CRAM adapters. Some specific questions are:

1) The JSON config for ExtendedVariantAdapter and VcfTabix adapter are identical. Is there a way to simply reference the existing VcfTabixAdapter schema by name here, rather than duplicate it? https://github.com/BimberLab/DiscvrLabKeyModules/blob/discvr-23.3/jbrowse/src/client/JBrowse/Browser/plugins/ExtendedVariantPlugin/ExtendedVariantAdapter/configSchema.ts

2) In the actual ExtendedVariantAdapter class, would it be reasonable to simply create an instance of VcfTabixAdapter in the constructor (or maybe configure(), actually), and then delegate to it? Code here: https://github.com/BimberLab/DiscvrLabKeyModules/blob/discvr-23.3/jbrowse/src/client/JBrowse/Browser/plugins/ExtendedVariantPlugin/ExtendedVariantAdapter/ExtendedVariantAdapter.ts. I dont believe that would require implementing very much in ExtendedVariantAdapter itself. I would need to implement the BaseFeatureDataAdapter interface. If this seems reasonable, if there a standard way to create an instance of a VcfTabixAdapter by name, where I could pass through the config given to ExtendedVariantAdapter?

Again, sorry if I'm missing something about the subadapter strategy that makes this a better fit.