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

Export common functionality #983

Closed
dvmoritzschoefl opened this issue Oct 18, 2023 · 7 comments · Fixed by #997
Closed

Export common functionality #983

dvmoritzschoefl opened this issue Oct 18, 2023 · 7 comments · Fixed by #997
Assignees
Labels
enhancement New feature or request

Comments

@dvmoritzschoefl
Copy link

We implemented a custom data fetcher for a track and therefore needed some functionality which was already in gosling, like functions to convert from absolute to relative genomic positions and vice versa. However these utility functions are not exported from the gosling library and therefore we had to rewrite/copy them, which is not optimal. It would be nice if all utility functions would be exported so they can be used from outside of gosling.

@dvmoritzschoefl dvmoritzschoefl added the enhancement New feature or request label Oct 18, 2023
@sehilyi
Copy link
Member

sehilyi commented Nov 15, 2023

Hi @dvmoritzschoefl! Thanks for the suggestion, and we fully agree that it will be useful to expose utility functions for developers. Since there are a large number of utility functions in Gosling.js, and we are still figuring out which utility functions we want to make public, we would need to expose selected functions having clear use cases for developers.

In your case, it seems like functions in /utils/assembly.ts (code) are the ones that you would need, which are:

  • getRelativeGenomicPosition
  • computeChromSizes
  • getChromInterval
  • getChromTotalSize
  • parseGenomicPosition

Can you confirm if these were the functions that you need to rewrite? Please also let me know if you have other utility functions in mind since that would be very helpful for us.

@sehilyi
Copy link
Member

sehilyi commented Nov 15, 2023

@manzt Would you have any general suggestions on this?

@dvmoritzschoefl
Copy link
Author

Yes @sehilyi these were exactly the functions we needed. In addition I also think there was one called sanitizeChrName for removing the chr prefix from names. Basically our use case was that we had an API which delivered relative genomic positions and for gosling we needed relative ones (or vice versa).

@sehilyi
Copy link
Member

sehilyi commented Nov 23, 2023

@dvmoritzschoefl I made a PR for this (#997). Please let me know if it reflect on your use cases!

@dvmoritzschoefl
Copy link
Author

@dvmoritzschoefl I made a PR for this (#997). Please let me know if it reflect on your use cases!

Yes that would cover everything we need. However most libraries would export it in a style like this

import { computeChromSizes, ... } from 'gosling.js/util'

This way auto import would still work (I think with the solution in the mentioned PR this would not be recognized by vscode)

@sehilyi
Copy link
Member

sehilyi commented Nov 29, 2023

This way auto import would still work (I think with the solution in the mentioned PR this would not be recognized by vscode)

Ah, good point. I followed how HiGlass supports utils functions (i.e., making utils an object that contains all utility functions). But, your suggestion seems to be closer to how we eventually want to support util functions when we make gosling.js a monorepo (#897):

import { computeChromSizes, ... } from '@gosling-lang/util';

Do you have any suggestions on how we can achieve your suggested syntax? i.e.,

import { computeChromSizes, ... } from 'gosling.js/util

(@manzt might have an idea!)

@manzt
Copy link
Member

manzt commented Nov 29, 2023

Ah just catching up here, I think I like the suggestion of having a separate package entrypoint for these utils in lieu of a separate package (i.e., gosling.js/utils). However, Gosling.js is currently bundled into a single JavaScript entrypoint when released to NPM.

Ideally we could set up the package.json to have:

{
  // ...
  "exports": {
    ".": "./dist/gosling.mjs",
    "./utils": "./dist/utils.mjs"
  }
}

However, this would require setting up out build to generate ./dist/utils.mjs. Maybe it's not that hard with all the changes from @etowahadams.

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

Successfully merging a pull request may close this issue.

3 participants