Invalidate require cache when requiring js files

[PEZ]

I made it so that we invalidate the module require cache when a js file is required. Dunno if that is very inefficient? Is it OK for a first version of this, you think, @borkdude? Or is it fine, generally?

[PEZ]

Now added a check for :reload before invalidating that cache entry.

[borkdude]

Do I understand correctly that :reload works for node_modules libraries and ./foo.js local JS files?

[PEZ]

I haven't tried it for node_modules, but I can do that. For local JS files it works. With no :reload, cache is used, with it, the cache entry is first killed causing a reload.

[PEZ]

Hmmm, I probably need to check for it is a file or a node_modules thing. I think JS uses a rule that file libs need to start in a way that distinguish them from node_modules. So ./foo or /Users/pez/foo. We could do the same. As it is now we also require that it is a full filename. Which I think is fine and simple to implement.

[borkdude]

@PEZ Here's how I did it in nbb:

https://github.com/babashka/nbb/blob/79bc68474afa9d79bb037613b36b1ce20b3acb4d/src/nbb/core.cljs#L231-L233

If the file starts with ./ then we do some massaging to the libname to resolve it against the current file, but else you don't need to do anything with it.

[borkdude]

It would be good to check what the actual entries in that cache are for 1) relative paths, 2) absolute paths, 3) node_module references ("axios"). Could you post that below?

[PEZ]

For js-files the entries are absolute paths. I shall have a check what they look like for node_modules...

[PEZ]

For posthtml-parser, after our require, I see these keys in the cache:

"/Users/pez/Projects/joyride-aoc/.joyride/node_modules/posthtml-parser/dist/index.js" 
"/Users/pez/Projects/joyride-aoc/.joyride/node_modules/posthtml-parser/dist/chunk.2UQLUWPH.js"

For a regular file it looks like so:

"/Users/pez/Projects/joyride-aoc/.joyride/src/aoc/day-1.js"

[PEZ]

I think it will get tricky to try reload node_modules, and not as useful. Shall we just document that this is not supported?

[PEZ]

@PEZ Here's how I did it in nbb:

https://github.com/babashka/nbb/blob/79bc68474afa9d79bb037613b36b1ce20b3acb4d/src/nbb/core.cljs#L231-L233

If the file starts with ./ then we do some massaging to the libname to resolve it against the current file, but else you don't need to do anything with it.

😄 Almost verbatim with what I have now. Cool. I'm getting borkified.

[PEZ]

Though, I do

        lib-path (if (.startsWith lib ".")
                   (path/resolve (path/dirname from-script) lib)
                   lib)

Which covers paths like ../foo/bar.js as well. Maybe nbb should too?

[borkdude]

I think you're right about .., PR welcome about that.

My point at the changes at require* are that it looks like you always do something with the cache, even if it's a normal node_modules library. So it goes like this: (if (path/isAbsolute "axios") ... (path/resolve (path/dirname from-script) "axios")). Which doesn't seem correct to me.

Btw, the resolved value at the end is the absolute file from a node library. This can be the value that is evicted from the cache.

[PEZ]

Now only js-files will obey the :reload option.

[PEZ]

Which doesn't seem correct to me

Because it wasn't. 😄 Tbh, I had forgotten about that we were loading node_modules in that function. But now it should be correct. At least according to my tests. If we by correct mean that we reload js-files, when asked to, but never try to reload node_modules.

[PEZ]

Btw, the resolved value at the end is the absolute file from a node library. This can be the value that is evicted from the cache.

Indeed, this is what resolved looks like when requiring posthtml-parser:

"/Users/pez/Projects/joyride-aoc/.joyride/node_modules/posthtml-parser/dist/index.js"

[PEZ]

But that's after the fact. To use it for invalidation we would need to keep a lookup between lib and resolved.

[borkdude]

resolved doesn't mean it's already loaded. that only happens in the last step (js/require ..). so you can still evict it from the cache.

[borkdude]

When :reload is true, you can just remove the resolved value from the cache. There is imo no lookup necessary.

[PEZ]

I'll have another look at it.

[PEZ]

Thanks! Now we use the resolved value and can skip all shenanigans with checking relative or absolute paths:

(defn require* [from-script lib {:keys [reload]}]
  (let [req (module/createRequire (path/resolve (or from-script "./script.cljs"))) 
        resolved (.resolve req lib)]
    (when reload
      (aset (.-cache req) resolved js/undefined))
    (js/require resolved)))

[borkdude]

I think you should still resolve a ./ or ../ file in relation to the current script

[borkdude]

It would certainly help if we had some tests for this, since CI passes while there are zero checks for this.

[PEZ]

I think you should still resolve a ./ or ../ file in relation to the current script

We still do that, right? We use from-script when we create the require object.

[borkdude]

And that also makes it work for ./ files? Then it seems ok! I assume you tested this locally, then LGTM ;).

[borkdude]

With this knowledge, I'd appreciate a PR in nbb which does have actually tests for this :-)

[PEZ]

I'm figuring about ways to test this. If I come up with any idea, I'll include it in this PR.

[PEZ]

I didn't really find a good path here. I think some kind of integration test would fit, but I don't know how to set it up in a good way. This particular function actually is a bit less involved than a lot of other functions, but it is still tricky with current directory and resolving stuff. The production code relies on vscode here, and regular node tests do not have that.

When we get to setting up some e2e tests, we shall remember to include testing this, unless we have found a nice way to integration test it. Then we have vscode to help us.

[PEZ]

And that also makes it work for ./ files? Then it seems ok! I assume you tested this locally, then LGTM ;).

I choose to act on this. 😄