Feature request: support .js and .mjs file extension for config files

[nnmrts]

I was quite surprised that vr only supports .ts after moving from a json config to a dynamic one. It should additionally support .js and .mjs, just like Deno does.

I'll add a PR for this.

[nnmrts]

PR for this: #97

[nnmrts]

PR for the docs: jurassiscripts/velociraptor-dot-run#1

[umbopepato]

Hey @nnmrts, thank you for your suggestion and PR!
Adding more configuration file formats was requested in the past (see #30). While I have nothing against supporting more configuration formats, there are some technical issues that should be addressed: apart from the speed of config resolving in the local filesystem (which can certainly be improved) that would become an issue for the progress of CLI installation helpers. Long story short: in order to resolve remote configurations we need to make 1 network request for each combination of name + extension. If we add .js and .mjs that would make 12 requests just to resolve a single config. We could restrict remote resolving to one name (either scripts or velociraptor) or to specific extensions to reduce the number of requests, but I think that would be very incoherent and unfair for the users that want to use the excluded configs.

[nnmrts]

Thanks, appreciate your answer! Really really sorry that I missed #30, usually I always search through the existing issues and PRs before creating one, somehow this time I forgot it.

I appreciate the insight you gave about network requests and I admit I didn't even think about that. But I still think it's not that big of an issue. Here comes my attempt to convince you. It's quite long but please bear with me.

Not really 12 most of the times

So first of all, it would be very very uncommon for vr to make 12 requests. The way the loader currently is implemented, the number varies between 1 and 12, while 12 requests would be the "worst case" of having a config file named velociraptor.mjs. People that are currently using scripts.ts won't even notice the change, because their option comes before js and mjs. The order would be:

scripts.yaml
scripts.yml
scripts.json
scripts.ts
scripts.js
scripts.mjs
velociraptor.yaml
velociraptor.yml
velociraptor.json
velociraptor.ts
velociraptor.js
velociraptor.mjs

So, as we can see, getting scripts.ts would just take 4 requests here.

The currently supported file extensions could theoretically even be more prioritized by flipping the two loops, so it becomes:

scripts.yaml
velociraptor.yaml
scripts.yml
velociraptor.yml
scripts.json
velociraptor.json
and so on...

Getting scripts.ts would take 8 requests here, but this way velociraptor.ts would only take 9 requests as well, and not 10 like in the example before.

12 is not a lot

So this was just an assumption, but I really wanted to test it and so I did. First, local file reads, here's a table of the average of 1000 runs of an equivalent to loadConfig:

config file name	average
scripts.yaml	0.2419 ms
scripts.yml	0.2660 ms
scripts.json	0.3201 ms
scripts.ts	0.3793 ms
scripts.js	0.4413 ms
scripts.mjs	0.5015 ms
velociraptor.yaml	0.5793 ms
velociraptor.yml	0.6982 ms
velociraptor.json	0.7068 ms
velociraptor.ts	0.7882 ms
velociraptor.js	0.8644 ms
velociraptor.mjs	1.0354 ms

Two observations can be made here:

1. The last, "worst-case" option, velociraptor.mjs does indeed take longer than the first, scripts.yaml. On average, it took 0.7935 ms longer to get it. Let's be generous and round that up to 1 ms. That's still an incredibly tiny amount of time difference and really just negligible.

2. All of the options take a really tiny amount of time in general. The average of these averages above is roughly 0.56 ms. Also negligible in my opinion.

Now, let's take a look at the network side of things. This uses a naive rewrite using fetch and response.text() directly replacing Deno.stat and Deno.readTextFile. 10 runs of this yields these averages:

config file name	average
scripts.yaml	50.3153 ms
scripts.yml	69.8752 ms
scripts.json	136.7741 ms
scripts.ts	153.6103 ms
scripts.js	200.6151 ms
scripts.mjs	217.7293 ms
velociraptor.yaml	307.8746 ms
velociraptor.yml	429.3652 ms
velociraptor.json	350.6256 ms
velociraptor.ts	402.9355 ms
velociraptor.js	410.9052 ms
velociraptor.mjs	486.5494 ms

Oof. Yeah, that's a lot compared to the local test. But if we actually use Promise.allSettled, the story looks very different (average of 100 runs):

config file name	average
scripts.yaml	94.8664 ms
scripts.yml	29.6065 ms
scripts.json	25.4140 ms
scripts.ts	22.1586 ms
scripts.js	22.4979 ms
scripts.mjs	22.5519 ms
velociraptor.yaml	26.8949 ms
velociraptor.yml	23.1016 ms
velociraptor.json	23.7147 ms
velociraptor.ts	36.8934 ms
velociraptor.js	26.8032 ms
velociraptor.mjs	22.9760 ms

There's practically no time difference between different options here. Yes, the scripts.yaml takes way longer than the other ones, but probably because it's the first use of fetch in the test file I wrote. I assume that's just some Javascript/Deno initialization thing. I guess I could avoid this by calling a random example fetch at the start of the test file, but yeah, there's no real reason why scripts.yaml shouldn't just take roughly 25 ms to fetch as well.

And I don't think 25 ms is a lot. Sure, it always depends on your internet connection and a lot of other factors, but how long a single request takes isn't really the point here, right? The thing we try to find out here is:
Does adding additional extensions make the whole loadConfig function noticeably slower? Basically: Will people using scripts.ts have a worse experience if we add scripts.js to vr?

Adding extensions has little effect

I wanted to test this directly and generally, so here we go. Let's use the function we used above, specify which extensions are allowed and compare different configurations of possible file extensions. These are the averages of 1000 local runs:

possible extensions	average
yaml	0.2143 ms
yaml,yml	0.2732 ms
yaml,yml,json	0.3294 ms
yaml,yml,json,ts	0.3888 ms
yaml,yml,json,ts,js	0.4402 ms
yaml,yml,json,ts,js,mjs	0.5140 ms

As we can see, there is indeed a difference between the current state yaml,yml,json,ts and my proposed addition yaml,yml,json,ts,js,mjs. It's 0.1252 ms. Really negligible I would say. Let's do the same thing online, this time we will just skip the naive way. Averages of 100 network runs:

possible extensions	average
yaml	21.8518 ms
yaml,yml	22.3355 ms
yaml,yml,json	25.3257 ms
yaml,yml,json,ts	28.1655 ms
yaml,yml,json,ts,js	24.4655 ms
yaml,yml,json,ts,js,mjs	27.2796 ms

It already took some time to do these 100 runs per possible extensions configuration but it's quite obvious that 100 runs is not enough to get a good average. yaml,yml,json,ts taking longer than yaml,yml,json,ts,js, like shown above, makes no real sense and is probably due to my internet or my disk being a bit slower while the test went through that part. But one thing is obvious: The order of magnitude by which these averages increase seems to be really really small. Going from yaml, one possible extension, to yaml,yml,json,ts,js,mjs, six possible extensions, just added 6 ms. And we can't even be sure if the reason for this is actually adding extensions. My CPU getting hotter or Chrome updating in the background probably had a bigger impact on these numbers.

Conclusion

I'm not a scientist and the methods described above are obviously not really scientific. However, I believe the results of my tests still point in a direction of "it doesn't really matter if we add .js and .mjs". I admit this was my initial assumption as well, so I obviously went into this biased. Consequently, I invite you, whenever you are able to sacrifice some time for this of course, to just try it out yourself by cloning this repository I created. It's a CLI with which you can reproduce the tests outlined in this comment, the readme file explains it all.

I also realize there are some implementation specific flaws with my loadConfig. For example, it currently loads or fetches the config file and just reads its text content. In the case of dynamic files, this isn't really what we want, calling await import(url) there would be more appropriate obviously. I didn't want to overcomplicate this even more than I already did. But I am confident that my tests still show the general impact of adding extensions, no matter if we try-catch import or try-catch Deno.stat or handle fetch promise rejections.

[umbopepato]

Wow, this is the super detailed performance investigation that I never had the time to do, thank you so much for the effort 😍
Making the network requests in parallel could be a good way of reducing the resolve time (also, since Deno natively supports AbortController we could try and cancel the other requests when the good one resolves). I was more worried about not overloading the registries but I guess this is still a fairly acceptable number (they probably serve many more requests when loading module dependencies).
Now, as you could probably tell from my terrible response time I'm super busy in this period of time (apologies) but I'll merge your PRs as soon as possible and I hope I'll be able to work on remote resolving in the coming weeks. Let me know If you want to help with that as well, I'll add you to the organisation in case 🙂