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 option to "reopen" closed portal session #32

Merged
merged 1 commit into from Jan 4, 2021

Conversation

rfhayashi
Copy link
Contributor

@rfhayashi rfhayashi commented Dec 27, 2020

Adding a new parameter to portal.api/open function that accepts a portal instance. If the instance is closed, a new one with the same session id is open. If the instance is still open, the function does nothing.

This can be useful for tooling built on top of portal since it does not need to keep track whether the browser window is open or not. A use case for that is integrating portal with org babel.

TODO:

  • Implement for node runtime
  • Implement for web runtime
  • ?Add portal parameter to api/close and api/clear functions?
  • Update docs

@rfhayashi
Copy link
Contributor Author

@djblue Opening this PR with what we discussed in slack. Will work on the TODO list after getting your initial feedback. Thanks!

([options]
(l/open options)))
(open nil options))
([portal options]
Copy link
Owner

@djblue djblue Dec 27, 2020

Choose a reason for hiding this comment

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

I think the weird thing here is that if I didn't pass options initially, now I need to pass nil as the second arg for the dispatch to work correctly. I wonder if instead of arity overloading we can check if you are passing options or an exiting session? What do you think @rfhayashi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my use this was not a big problem because I ended up having only one call. It does look weird though. I normally don't like testing the type of the input (unless there is clear polymorphism) because it adds complexity. But maybe in this case it is acceptable. Do you prefer that way? I'm going your direction on that one.

Copy link
Owner

Choose a reason for hiding this comment

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

I think you can treat both arguments as maps and you would essentially be dispatching off of the existence of a :session-id. I prefer that over passing in a nil arg.

Copy link
Owner

Choose a reason for hiding this comment

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

I think having the launcher functions take two arguments is cool, but I would rather not expose that via the api. Is that a problem for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that if one wanted to pass options they would need to pass it every time open is called, but now I see that the options get merged into an atom, so that makes sense to me now. I removed the two arguments arity from the api.

@djblue
Copy link
Owner

djblue commented Dec 27, 2020

@rfhayashi This looks really good so far!

I think you can check the web runtime off the list as currently you can only have one session anyway.

@djblue
Copy link
Owner

djblue commented Dec 31, 2020

@rfhayashi This PR looks like it's getting pretty close to merge. I left one additional comment and once that's resolved we should be good.

this makes it possible to "reopen" a closed portal session
@rfhayashi rfhayashi marked this pull request as ready for review January 3, 2021 12:20
@rfhayashi
Copy link
Contributor Author

@djblue We could potentially also add the portal parameter to close and clear, but I guess we could do that in a later PR. So I think we're good to go, but let me know if you find anything else.

@djblue djblue merged commit db11474 into djblue:master Jan 4, 2021
@rfhayashi rfhayashi deleted the session-open branch January 14, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants