-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Loader class #114
Loader class #114
Conversation
src/dataloader.ts
Outdated
const sourcePath = targetPath + ext; | ||
const path = join(sourceRoot, sourcePath); | ||
try { | ||
await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Can we add a script to purge the cache? |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems in conflict with the PR that I created here: #116, or else there we will be a lot to reconcile at least.
src/dataloader.ts
Outdated
} | ||
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems incomplete or missplaced? "...it is the path..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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”.
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"]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did you remove argv0 here? Isn't it needed? You also removed the TODOs without doing them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/dataloader.ts
Outdated
const sourcePath = targetPath + ext; | ||
const path = join(sourceRoot, sourcePath); | ||
try { | ||
await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/dataloader.ts
Outdated
const sourcePath = targetPath + ext; | ||
const path = join(sourceRoot, sourcePath); | ||
try { | ||
await access(path, ext === ".sh" ? constants.X_OK : constants.R_OK); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
I think for now it’s just |
This…
Adopts access instead of stat to check if loaders are readable or executableWe can now cache
docs/.observablehq/cache
in CI during build.