move libs/** submodules into source tree

sdumetz commented 2 weeks ago

Hi,

This PR contains a lot of changes I wish to do to try to clean up duplicate/unused/unpractical code but most of those are mostly independant from each-other. It should be seen primarily as a pool of ideas, of which any number can be merged.

I tried to make readable commit messages but just to give a quick summary :

b5183516d61f418a1184a2dbac23943888e93780..88ecb87e0f0c90869eaa584c6aa109e90c94a3df: I moved everything that was in submodules under the libs/** folder to be checked-out in the repository.
- I squashed each one individually to merge in the source tree. It would be possible to keep the entire commit history but that would make the log much more verbose. Here I think the original code attribution is clear and the history stays readable.
115de381a1fc5a98f5eb10dd14274fd31d8bc081 remove untouched files automatically
806fd521da61d37a791805cb730f578f3489c945 remove dummy tests from the ff-* suite (left real tests untouched)
30a503976948362b6e7fdaa171460f1d6556a263 move everything under libs/**to source/**
- I think this is more readable, more in line with how most projects manage their folder structure. Might be a matter of taste?
- f764fb72e764ee275f76d2bdd69655140e6732a4 remove custom parseUrlParameter function
  - I don't like homegrown string parsers for such things when they can be avoided. QueryStrings are harder than they sound. Here it's not a security risk, but still. And less code.
- 90cd054d6e28d4d2f9543c7e720f0da5f2af27b6..558f960064282e3fd49fcf9b1097efdccfa87582 remove all fetch.{text,json,file} references
  - It was mostly doing nothing more than fetch's defaults (which was already used in most place).
  - JSONWriter: Everything else is PUT in place without being routed through a writer, I couldn't see why JSON should be different. It makes CVAssetWriter more consistent.

sdumetz commented 1 week ago

Working on the follow-ups for https://github.com/Smithsonian/dpo-voyager/pull/288, I want to update three.js to r0.160+, which breaks down quite a lot of things from the ff-* libs. I've gone through the changelog and am ready to do the changes once we agree on a course of action on this.

gjcope commented 1 week ago

Do you have an example of another project that is managing folders like https://github.com/Smithsonian/dpo-voyager/commit/30a503976948362b6e7fdaa171460f1d6556a263? I'm all for moving the used code over, but I think the /lib organization makes it more obvious that these aren't Voyager libraries. But might feel otherwise if you have an example.

sdumetz commented 1 week ago

It depends if you think of the ff-* code as voyager's code (as nobody else uses it), in this case, gltf-transform comes to my mind, with some (more or less) internal packages and an external cli package.

If you don't consider them "Voyager code", it is better to show this and keep them as "vendors" in a separate folder.

Either way it's OK for me, I'll edit the PR to cut off the folder rename if you think it better reflects the ownership of this code.

gjcope commented 1 week ago

I don't consider the ff-* code as Voyager's code. They were developed outside of the project and I believe are (or at least were) used by its developer in other projects.

sdumetz commented 1 week ago

I dropped the commit that moved everything to source/.

gjcope commented 1 day ago

Working on merging this in now. Some of it seems like preference, but I'm ok with it. Still need to do a little testing, but the main issue I'm seeing right now is that a lot of the library cleanup you did doesn't seem to be coming through when I pull this branch. Something to do with removing that last commit?

There are also a few ff-* file updates for this next release so not sure those will get merged in appropriately...

sdumetz commented 1 day ago

Some of it seems like preference

Glad if you like it, there is clearly some subjective choices in here.

The submodules removal uses exotic merges that should come through cleanly but they might not. I don't know how a reference change introduced in a previous commit would get handled.

Do you have a working branch where I could get a look ? You just merged this branch over rc43?

gjcope commented 1 day ago

I hadn't yet because I was unsure how to merge the other ff-* library updates I made, but I can just redo them. I will merge with rc-43 tomorrow and let you know.

gjcope commented 16 hours ago

https://github.com/Smithsonian/dpo-voyager/tree/dev-cleanup seems to somehow be missing the moving of the ff-* libs

Smithsonian / dpo-voyager

move libs/** submodules into source tree #284