code style

[mbostock]

Async code review for some recent changes:

• Organize imports
• Use “maybe” prefix for methods that may return null
• For “maybe” methods, return null instead of undefined (implies known rather than unknown)
• Rename findLoader to maybeLoader; return null instead of empty object to be more explicit
• Move the join(root, path) out of maybeLoader and to the caller
• Rename runCommand to runLoader to consistency (in the future, consider a loader.run method?)
• Use an async function to avoid uncaught rejections in runLoader
• Avoid mutation in resolveDiffs
• Avoid extra blank link when rendering HTML when there’s no database.js modulepreload
• Remove unused error binding in try-catch
• Remove unused url.toString()
• Remove unnecessary in test

It looks like there aren’t any tests for data loaders yet? We should think about how we could add those. I haven’t tested the above changes (other than running the tests).

[wiltsecarpenter]

I'm used to seeing "maybe" on conditional updates, not on get/find method, and when it is used it seems like it should modify a verb-object, like "maybeCommit" or "maybeCommitNotebook". We're free to create whatever convention we want, but "maybeStat" and "maybeLoader" sound strange to me.

[mbostock]

I think “findLoader” is a fine name, and I’m happy to revert that back. The main thing is that I wanted it to return null or undefined instead of an empty object (because this provides a more obvious way of checking existence). And I think “find” reasonably implies that it can return null or undefined (like array.find).

For “maybe” stat, it’s for parity with the underlying stat method, which at least in my experience is often used as a verb, as in “stat’ing a file”. We’ve used the “maybe” pattern extensively in Plot, but mostly for parsing/normalize user input to some trusted representation. I like the “maybe” pattern here because it indicates conditionality: you might not get the stats returned.

[wiltsecarpenter]

It could really be any kind of command I suppose, not just a loader.

If we want the naming to be consistent, we could change "commandPath" to loaderPath

[mbostock]

If we want to make further changes here, I’d consider having findLoader return a Loader implementation with a run method. Then the caller would say loader.run instead of runLoader, and you wouldn’t have to pass in the command path again.

[wiltsecarpenter]

Do these changes fix the bug that Fil saw with multiple FileAttachments calls for the same file?

[Fil]

It does! ref. #89 (comment) ; I've built the repro and can confirm that it crashes on main and doesn't crash with this branch.