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

Implement reload-all by regex #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jpmonettas
Copy link

First try to implement reload-all by regex as described in #10

Per requirements on the issue this PR :

  • doesn't change much code
  • shouldn't change the perf of current functionality

The main thing here is clj-reload/reload-all. As mentioned in the docstring it will only reload already loaded namespaces that have at least one var. The one var limitation is because I don't know any other way of getting the files to all namespaces, but I also think this is not an important limitation since reloading a namespace with no vars isn't probably useful.

There are a couple of small changes to the existing code like :

  • making clj-reload.core/ns-load work on resources urls as well as files.
  • clj-reload.parse/read-file try catch on the implementation signature, so both sigs are covered
  • make clj-reload.util/ns-load-file require only a file-name instead of a ^File object
  • A bunch of tests namespaces now have a dummy var with the same ns name, for testing purposes

I don't expect this to be merged as is but we have something to start with.

@tonsky
Copy link
Owner

tonsky commented Jun 28, 2024

Thanks, I’ll take a look soon

@tonsky
Copy link
Owner

tonsky commented Jul 2, 2024

Hi! I took a look, and have couple of suggestions:

  1. Unload/reload logic is pretty complex, I would prefer not having it in two places. What if reload-all shared implementation with reload as much as possible?
  2. This is also important because reload-all should affect state as well IMO
  3. To share the code, simplest way seems to be to introduce new option, :force #"pattern" (and maybe :force :all for symmetry)
  4. All scan/unload/reload should become jar-aware as well. That will require redoing all the state (:files etc will have to be replaced with what? Something like paths? URLs? I’d prefer plain strings if possible, java gives us a convention of "jar:file:/.../!.../..." and "file:/.../...")
  5. This concerns me, because now we have to read/parse/sort all library namespaces as well. But maybe just once? Since they can’t change, we’ll need to cache results of parsing for them.
  6. If we implement caching, the only thing that will affect performance when we add jars will be sorting. I’d love to see some numbers, what it takes to sort only direct source namespaces on a project like logseq, compared to sorting direct namespaces + all the dependencies on a classpath.
  7. I’d love to see numbers of how long it takes to scan and parse all the dependencies for a big project like logseq, too. Even if we’ll have to do it once, startup time still counts. That will determine if we want jars always or only explicitly enabled as an option
  8. For me, going from namespace -> vars -> file -> resource feels weird. Can we list all resources directly? And select .clj from that? Or scan classpath? What does tools.namespace do? I would love to be able to work with namespaces without var as well.

I know it sounds like much, that’s why I was resistant to implementing this myself :) But should be doable. I suggest we start with some measurements though. To decide if this should be “enabled for everyone, always” or as an option. That will affect API design

@jpmonettas
Copy link
Author

Sure. I didn't went the integrated route in part because it was more complex, and in part because it would mess with the currenty functionality which I was trying to avoid (so you didn't need to worry about it).

I was trying to design it as a separate utility that clj-reload provided, but I agree that having an integrated api is much nicer.

Also agree that for going the integrated path we need some numbers for the perf.

namespace -> vars -> file -> resource feels weird

I agree. We can try directly from resources, but then we need to somehow go from resources to namespaces, and then filtering out what isn't loaded. I don't think it is nice to tell it to reload #"ring*", and suddenly you have loaded a bunch of ring something namespaces that weren't loaded before.

@tonsky
Copy link
Owner

tonsky commented Jul 2, 2024

I agree. We can try directly from resources, but then we need to somehow go from resources to namespaces, and then filtering out what isn't loaded. I don't think it is nice to tell it to reload #"ring*", and suddenly you have loaded a bunch of ring something namespaces that weren't loaded before.

But that’s already how it works. It scans everything but only loads what is already loaded

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