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

Create packages/product-core containing shared code between the various products #3661

Merged
merged 44 commits into from May 9, 2023

Conversation

rbuels
Copy link
Contributor

@rbuels rbuels commented Apr 24, 2023

Work in progress DRY-ing up shared code among the products. Primarily the session models in this first phase.

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Apr 24, 2023
@rbuels
Copy link
Contributor Author

rbuels commented Apr 24, 2023

@cmdcolin the plan for this PR is to split up the sessionModelFactory files (which are more than 1000 lines each) into multiple files by area of functionality (assemblies, connections, etc), and to factor out the duplicated parts of them into packages/product-core.

Are you OK with that plan?

The motivation for this is that the huge files and code duplication are making the session models increasingly hard to work on.

@cmdcolin
Copy link
Collaborator

i'm fine with it if it seems like it works right. I have previously tried to do this but found it can be difficult to properly "carve" out portions of the state model but if it works that's great (my previous attempt stalled #3206)

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #3661 (acec95e) into main (21e2cd0) will increase coverage by 0.95%.
The diff coverage is 51.89%.

@@            Coverage Diff             @@
##             main    #3661      +/-   ##
==========================================
+ Coverage   62.61%   63.57%   +0.95%     
==========================================
  Files         894      915      +21     
  Lines       30246    29912     -334     
  Branches     7322     7264      -58     
==========================================
+ Hits        18939    19016      +77     
+ Misses      11124    10713     -411     
  Partials      183      183              
Impacted Files Coverage Δ
...kages/core/assemblyManager/assemblyConfigSchema.ts 90.00% <ø> (ø)
...eElementTypes/models/BaseConnectionModelFactory.ts 75.00% <ø> (ø)
...re/pluggableElementTypes/models/baseTrackConfig.ts 58.33% <ø> (ø)
packages/core/util/types/index.ts 70.96% <ø> (ø)
products/jbrowse-desktop/src/JBrowse.tsx 5.00% <0.00%> (ø)
products/jbrowse-desktop/src/StartScreen/util.tsx 0.00% <0.00%> (ø)
products/jbrowse-desktop/src/indexJobsModel.ts 1.31% <ø> (ø)
products/jbrowse-desktop/src/jbrowseConfig.ts 66.66% <ø> (ø)
...ircular-genome-view/src/createModel/createModel.ts 37.50% <ø> (ø)
...-genome-view/src/createModel/createSessionModel.ts 34.37% <0.00%> (+22.88%) ⬆️
... and 35 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rbuels rbuels added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Apr 27, 2023
@rbuels rbuels marked this pull request as ready for review May 8, 2023 21:33
@@ -154,7 +154,8 @@
"test": true
},
"rules": {
"import/no-extraneous-dependencies": "off"
"import/no-extraneous-dependencies": "off",
"@typescript-eslint/no-non-null-assertion": "off"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allows non-null assertions in test code

const JBrowseRootModel = JBrowseRootModelFactory(
pluginManager,
sessionModelFactory,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to how the root model is instantiated was done to break a dependency cycle between the root, session, and jbrowse model types

@rbuels
Copy link
Contributor Author

rbuels commented May 8, 2023

@cmdcolin This might be getting close to ready to merge, I think. I don't know what effect it has on the doc generation you worked on though

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 9, 2023

awesome, i'll check do a quick review. not too worried about docs for now, itll probably break e.g. the rootmodel page into multiple pages with the mixins but thats fine with me

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 9, 2023

i'd say let's get it merged :) we can iterate on main

@rbuels
Copy link
Contributor Author

rbuels commented May 9, 2023

I'm fine with that, merge when ready

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 9, 2023

one thing i noticed, is that "BaseSession" might be mixed in multiple times

Screenshot from 2023-05-09 10-53-49

this doesn't appear to cause any issue though

@cmdcolin cmdcolin merged commit 1fe312f into main May 9, 2023
11 of 12 checks passed
@cmdcolin cmdcolin changed the title create packages/product-core containing shared code between the various products Create packages/product-core containing shared code between the various products May 9, 2023
@cmdcolin
Copy link
Collaborator

cmdcolin commented May 9, 2023

woooo merged. great work :) thanks @rbuels

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.

None yet

2 participants