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

Use the JsCode object from the js_loader package #2081

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hansthen
Copy link
Collaborator

This PR is based on a discussion in #1942.

In that issue, the user wanted to load JsCode objects from a javascript file, instead of embedding javascript code as strings in a python file. The advantages are that users can use linters, syntax highlighting and autocomplete on the JavaScript code.

The js_loader package allows this. It provides an import hook that will parse Common JS source files and exposes exported functions as JsCode objects. It allows users to do this:

from js_loader import install_js_loader
install_js_loader()

from path_to_js_module import js_module
  
js_module.my_event_handler # can now be used as an event handler in a Folium map.

@hansthen hansthen requested a review from Conengmo January 16, 2025 15:14
Copy link
Member

@Conengmo Conengmo left a comment

Choose a reason for hiding this comment

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

First question, why not add it to the requirements.txt, but add it 'manually' to the Github Actions workflows and setup.py?

As it is, I don't see the benefit of this PR. We exchange 11 lines of code for a new mandatory dependency. I know you made that library, so it's not that I don't trust you. But seen from Folium, we'd give up control of this part to just you and whoever is in the 'streamlit-community' org, which we don't know.

The difficulty with Branca being a separate library has shown that there's a downside to splitting interwoven functionality off. I don't want to repeat that with JsCode.

So having an external library that helps with loading Javascript files? Sounds good! Moving a class that is used everywhere throughout Folium to a separate library? Maybe not a good idea.

@hansthen
Copy link
Collaborator Author

hansthen commented Jan 17, 2025

First question, why not add it to the requirements.txt, but add it 'manually' to the Github Actions workflows and setup.py?

The js_loader library is not hosted on conda. So adding it to requirements.txt will raise an error when running the tests, because it tries to install js_loader from there.

As it is, I don't see the benefit of this PR. We exchange 11 lines of code for a new mandatory dependency. I know you made that library, so it's not that I don't trust you. But seen from Folium, we'd give up control of this part to just you and whoever is in the 'streamlit-community' org, which we don't know.

Fair enough, but I think it is not so bad in practice. See my comments below.

The difficulty with Branca being a separate library has shown that there's a downside to splitting interwoven functionality off. I don't want to repeat that with JsCode.

Branca is much more tightly interwoven with Folium than JsCode and Folium. JsCode is not structurally related to Folium at all. In fact Folium existed just fine without it until December 2023. JsCode is much more comparable to using xyzservices or geopandas. If I had written js_loader first and only then used the JsCode from it in Folium would you feel the same?

So having an external library that helps with loading Javascript files? Sounds good! Moving a class that is used everywhere throughout Folium to a separate library? Maybe not a good idea.

I see your concern about losing control over a key class. (It is also flattering to see how quickly JsCode became core Folium.) However, I also think loading event handlers from javascript files is useful for Folium. Do you have suggestions how we can achieve the second and avoid the drawbacks of the first?

One suggestion from me is that we can make js_loader part of the Folium world, so you don't lose control. Perhaps you have other suggestions?

@Conengmo
Copy link
Member

Conengmo commented Jan 18, 2025

The js_loader library is not hosted on conda

Ah I understand. I guess we should get it on Conda if we want to adopt it.

JsCode is much more comparable to using xyzservices or geopandas

That's an interesting comparison. I feel like the contact surface with xyzservices is pretty limited in comparison, and also not expected to change. Geopandas is not even an required dependency. I see JsCode being used in much more places, and also being something we might want to adapt going forward.

It is also flattering to see how quickly JsCode became core Folium

Definitely true, it's been a great idea!

I also think loading event handlers from javascript files is useful for Folium

Agreed. This PR just moves JsCode to another repo, so I'm mainly responding to that. I'd be very interested to instead learn more about how it would look like to support the extended behavior you mention here. Maybe we can discuss some code snippets that achieve that?

If we know what behavior we want to include, we can then decide where we ideally want to place those. Some options:

  • include them in the Folium repo
  • include them in the Branca repo (makes sense maybe since it's general templating behavior)
  • make a new repo under python-visualization
  • a repo in an external org
    • since that would be a required dependency, I would have some requirements for that in terms of stability and availability.

give an example how we can load javascript from
javascript files.
so the js_loader example will work
@hansthen
Copy link
Collaborator Author

hansthen commented Jan 18, 2025

I added some documentation to the advanced guide to show someone could load JsCode from a javascript file.

Moving JsCode was mainly done to keep the type checker happy. In case our users load JsCode from js_loader I don't want them to see a mypy error about it. We could just ask our users to ignore type checking in this case. "If Duck Typing is good enough for Guido, it should be good enough for you!"

Alternatively, we could also work around that by using a mypy protocol wherever we now expect a folium.utilities.JsCode object as a parameter. I have no experience with protocols, but they look similar to a Java interface (but in Python). That way our users can use js_loader.JsCode since they implement the same protocol as JsCode.

and JsCode like objects that come from external sources.
@hansthen
Copy link
Collaborator Author

@Conengmo I implemented typing using a Protocol. Apparently we only need the js_code attribute. This way we do not rely on js_loader for JsCode. I still need it for the tests though. Let me know if you like this better.

@hansthen hansthen requested a review from Conengmo January 18, 2025 19:43
@Conengmo
Copy link
Member

Sorry Hans I'm a bit overwhelmed at the moment, I do want to take a moment to properly understand this, but I don't have the space for it right now. But soon!

@hansthen
Copy link
Collaborator Author

hansthen commented Jan 28, 2025

Sorry Hans I'm a bit overwhelmed at the moment, I do want to take a moment to properly understand this, but I don't have the space for it right now. But soon!

No problem sir. We all have lives outside of Folium with work and family obligations.

I am not particularly attached to this change either. So its perfectly fine with me if you just reject it as not worth the effort.

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.

2 participants