Loader class #114

mbostock · 2023-11-08T01:04:48Z

This…

Supersedes dataloaders tests #110
Supersedes ignore executable bit #121
Fixes Implement .js and .ts data loader support #96
~~Adopts access instead of stat to check if loaders are readable or executable~~
Adds a Loader class with Loader.find factory and loader.load method
Moves the cache freshness test into loader.load
Fixes If an inconsistent outputPath is passed to runCommand, the requested file may not be generated #95
Fixes The cacheRoot should be relative to the current working directory, not where the cli is installed #98
Fixes Data loaders caching during build #100

We can now cache docs/.observablehq/cache in CI during build.

Fil · 2023-11-08T15:22:59Z

src/dataloader.ts

+      const sourcePath = targetPath + ext;
+      const path = join(sourceRoot, sourcePath);
+      try {
+        await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK);


Suggested change

await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK);

await stats(path);

I don't think we should check X_OK for shell scripts nor for R_OK in any case; a check on file existence seems to be enough and corresponds to the desirable experience. If the loader is not readable by our build script or dev server, I'd rather let the error happen visibly than silently skip the loader. (See also #121 re: the x bit.)

You removed the error message that I had in here originally for the executable bit. That led me at least to spend time debugging a loader that didn't have it set correctly. It seemed better to me to warn the user if they have something that looks like they meant it to work but it doesn't, rather than failing silently.

I’ve incorporated @Fil’s suggestion so that the executable bit is no longer required (invoking via sh). If we did want to run the program directly, then I agree it’s better to error if the executable bit is not set, rather than ignoring the data loader — so instead of trying to check the bits beforehand, we simply run the loader and let it fail.

src/dataloader.ts

Fil · 2023-11-08T15:46:05Z

Can we add a script to purge the cache?

wiltsecarpenter

This seems in conflict with the PR that I created here: #116, or else there we will be a lot to reconcile at least.

wiltsecarpenter · 2023-11-08T17:31:50Z

src/dataloader.ts

-}
+  /**
+   * Any args to pass to the command. For a JavaScript or TypeScript loader, it
+   * is the path to the loader script relative to the current working directory.


This comment seems incomplete or missplaced? "...it is the path..."

Seems correct to me? The “it” refers to the “args to pass to the command”, and the singular arg for a JavaScript or TypeScript loader is “the path to the loader script relative to the current working directory”.

src/dataloader.ts

wiltsecarpenter · 2023-11-08T17:38:21Z

src/dataloader.ts

+      await prepareOutput(cachePath);
+      const cacheFd = await open(cachePath, "w");
+      const cacheFileStream = cacheFd.createWriteStream({highWaterMark: 1024 * 1024});
+      const subprocess = spawn(this.command, this.args, {windowsHide: true, stdio: ["ignore", "pipe", "inherit"]});


What did you remove argv0 here? Isn't it needed? You also removed the TODOs without doing them?

The argv0 wasn’t necessary in my testing. Is it necessary? I don’t understand what it is for.

The cwd TODO asked whether we need to change the current working directory, and I think using the existing current working directory is the expected behavior.

The timeout and signal comments weren’t marked as TODO, but we can file an issue if we think it’s important to add that functionality. I think I tend to prefer issues over TODO comments since the latter are harder to see in one place.

wiltsecarpenter · 2023-11-08T17:45:48Z

src/dataloader.ts

+      const sourcePath = targetPath + ext;
+      const path = join(sourceRoot, sourcePath);
+      try {
+        await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK);


I get that it is more convenient to not require a .js to be executable, but using node or tsx as the command also has implications that I wonder about. Is it 100% always the case that .js implies that there is a node command in the current path that takes no arguments to run the script?

That’s the contract that we’re providing, yes. If people want some different behavior, they can use the .sh extension as the more general alternative.

wiltsecarpenter · 2023-11-08T17:47:25Z

src/dataloader.ts

+      const sourcePath = targetPath + ext;
+      const path = join(sourceRoot, sourcePath);
+      try {
+        await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK);


You removed the error message that I had in here originally for the executable bit. That led me at least to spend time debugging a loader that didn't have it set correctly. It seemed better to me to warn the user if they have something that looks like they meant it to work but it doesn't, rather than failing silently.

mbostock

I will restore the tempfile behavior as before, but will make the tempfile name include the pid so that multiple processes won’t interfere with each other (as when running observable build and observable preview simultaneously).

mbostock · 2023-11-08T19:36:48Z

Can we add a script to purge the cache?

I think for now it’s just rm -rf docs/.observablehq/cache. We could add an observable clean command, I suppose, but you’d still have to pass it the source root potentially, so I don’t see it as adding a lot of value on top of documenting where the cache lives. And in the case of GitHub CI caching, for example, we’re also going to depend on a known location for the cache folder we can pre-populate it prior to running observable build.

loader class

07d4e8d

mbostock requested review from Fil and wiltsecarpenter November 8, 2023 01:04

This was referenced Nov 8, 2023

ignore executable bit #121

Closed

When a data loader fails, unlink any previously cached output. #122

Closed

Fil requested changes Nov 8, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

wiltsecarpenter reviewed Nov 8, 2023

View reviewed changes

Fil mentioned this pull request Nov 8, 2023

colorful logs for data loaders #111

Closed

existsSync instead of access; sh

9aeea91

Fil mentioned this pull request Nov 8, 2023

dataloaders tests #110

Merged

mbostock commented Nov 8, 2023

View reviewed changes

mbostock added 4 commits November 8, 2023 11:18

500 on loader error

e4dc91c

process-specific temp file

076f592

avoid unhandled rejection

4275638

ignore errors when watching files

58ddb38

Fil approved these changes Nov 8, 2023

View reviewed changes

mbostock added 2 commits November 8, 2023 13:41

tweak comments; private fields

7322e77

Merge branch 'main' into mbostock/loader-class

af49ec3

mbostock enabled auto-merge (squash) November 8, 2023 22:07

mbostock merged commit 9dace2a into main Nov 8, 2023
1 check passed

mbostock deleted the mbostock/loader-class branch November 8, 2023 22:07

mbostock mentioned this pull request Nov 8, 2023

Add data loader caching to build command #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loader class #114

Loader class #114

mbostock commented Nov 8, 2023 •

edited by Fil

Fil Nov 8, 2023

wiltsecarpenter Nov 8, 2023

mbostock Nov 8, 2023

Fil commented Nov 8, 2023

This comment was marked as resolved.

wiltsecarpenter left a comment

wiltsecarpenter Nov 8, 2023

mbostock Nov 8, 2023

wiltsecarpenter Nov 8, 2023

mbostock Nov 8, 2023

wiltsecarpenter Nov 8, 2023

mbostock Nov 8, 2023

wiltsecarpenter Nov 8, 2023

mbostock left a comment

mbostock commented Nov 8, 2023

	await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK);
	await stats(path);

Loader class #114

Loader class #114

Conversation

mbostock commented Nov 8, 2023 • edited by Fil

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fil commented Nov 8, 2023

This comment was marked as resolved.

wiltsecarpenter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbostock left a comment

Choose a reason for hiding this comment

mbostock commented Nov 8, 2023

mbostock commented Nov 8, 2023 •

edited by Fil