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 extension point for session model #2120

Closed
wants to merge 4 commits into from

Conversation

elliothershberg
Copy link
Member

This is a tiny PR from an idea that @cmdcolin and I worked on. It is related to #2032, but is for the session model, instead of the state model on a display type. This was motivated by work on #2080 .

Here, before the session model is returned from the model factory, it checks if any plugins have an extendSession method defined. If it does, it updates the session model with the result of that function.

For example:

extendSession(sessionModel: IAnyModelType) {
  return types.compose(
    sessionModel,
    types.model('HelloWorld', { foopy: types.optional(types.string, 'lol') }),
  )
}

Would extend the session with the new attribute foopy.

We also investigated doing something similar for the LGV menu, but ran into some issues so stopped here for now. Would be curious about your thoughts @rbuels and @garrettjstevens .

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jul 9, 2021
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #2120 (1e65a98) into main (b223419) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2120   +/-   ##
=======================================
  Coverage   62.02%   62.02%           
=======================================
  Files         476      476           
  Lines       22789    22793    +4     
  Branches     5350     5350           
=======================================
+ Hits        14134    14138    +4     
  Misses       8374     8374           
  Partials      281      281           
Impacted Files Coverage Δ
packages/core/Plugin.ts 100.00% <100.00%> (ø)
products/jbrowse-web/src/sessionModelFactory.ts 59.32% <100.00%> (+0.25%) ⬆️
packages/core/util/index.ts 79.89% <0.00%> (ø)

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 b223419...1e65a98. Read the comment docs.

@garrettjstevens
Copy link
Collaborator

What would be a use case for this? If you're adding a pluggable element (like a view or widget), it has its own model that it can store things on, and that model will be in the session, so I'm not sure why we need an extra way to extend the session model.

@elliothershberg
Copy link
Member Author

What would be a use case for this? If you're adding a pluggable element (like a view or widget), it has its own model that it can store things on, and that model will be in the session, so I'm not sure why we need an extra way to extend the session model.

The use case this was written for was that when I tried to store bookmarks in the widget's model in #2080 , the values didn't persist. I may have been doing something wrong?

I think I'm going to close this draft in favor of a more general extension points implementation @rbuels and I just worked on, which can accomplish this also.

@elliothershberg
Copy link
Member Author

Closed in favor of #2140

@elliothershberg elliothershberg deleted the 2032_extension_points branch July 19, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants