Skip to content
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

fix: return undefined from FetchCacher#load on NotFound #64

Merged

Conversation

magurotuna
Copy link
Member

@magurotuna magurotuna commented Oct 17, 2024

This makes FetchCacher#load return undefined when the file fetcher returns NotFound error in order to align it with Deno CLI's implementation:

let error_class_name = get_error_class_name(&err);
match error_class_name {
  "NotFound" => Ok(None),
  "NotCached" if options.cache_setting == LoaderCacheSetting::Only => Ok(None),
  _ => Err(err),
}

With this patch, deno_emit can bundle source code containing imports from JSR.

Let's say we have a bundle target script as follows:

import { add } from "jsr:@magurotuna/test-add";

console.log(add(1, 3));

Without this patch, running bundle function from deno_emit on this script gives:

error: Uncaught (in promise) Error: Unable to output during bundling: load_transformed failed: failed to analyze module: failed to resolve jsr:@magurotuna/test-add from file:///Users/yusuke/Repo/github.com/magurotuna/deno_emit/js/maguro_add.ts: Cannot resolve "jsr:@magurotuna/test-add" from "file:///Users/yusuke/Repo/github.com/magurotuna/deno_emit/js/maguro_add.ts".
      const ret = new Error(getStringFromWasm0(arg0, arg1));
                  ^
    at __wbg_new_28c511d9baebfa89 (file:///Users/yusuke/Repo/github.com/magurotuna/deno_emit/js/emit.generated.js:563:19)
    at <anonymous> (wasm://wasm/010ad402:1:3297307)
    at <anonymous> (wasm://wasm/010ad402:1:358125)
    at <anonymous> (wasm://wasm/010ad402:1:1823927)
    at <anonymous> (wasm://wasm/010ad402:1:2937828)
    at __wbg_adapter_46 (file:///Users/yusuke/Repo/github.com/magurotuna/deno_emit/js/emit.generated.js:247:6)
    at real (file:///Users/yusuke/Repo/github.com/magurotuna/deno_emit/js/emit.generated.js:231:14)
    at ext:core/01_core.js:291:9
    at eventLoopTick (ext:core/01_core.js:175:7)

With this patch, it can generate:

function add(a, b) {
    return a + b;
}
console.log(add(1, 3));

Ref denoland/deno_emit#167

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test? That said, it looks like the code already handles this, but you are encountering an error. Where is this error surfacing from?

cache.ts Outdated Show resolved Hide resolved
return this.#fileFetcher.fetchOnce(url, { cacheSetting, checksum });
return this.#fileFetcher.fetchOnce(url, { cacheSetting, checksum })
.catch((e) => {
if (e instanceof Deno.errors.NotFound) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this code down into fileFetcher.fetchOnce instead? It looks like it's already handled for 404. Actually, it looks like the code is handled in fetchLocal too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this here, because in Deno CLI the same kind of error handling is performed in load method, instead of in file_fetcher. See https://github.com/denoland/deno/blob/1a0cb5b5312941521ab021cfe9eaed498f35900b/cli/cache/mod.rs#L372-L426, which is Loader trait impl for FetchCacher, where file_fetcher.fetch_no_follow_with_options is called, followed by the error handling in unwrap_or_else checking if the error is NotFound.
So I think that putting e instanceof Deno.errors.NotFound here would be more consistent.

@magurotuna
Copy link
Member Author

Can you add a test?

Done 74da9e6

That said, it looks like the code already handles this, but you are encountering an error. Where is this error surfacing from?

Not sure exactly, but the approach I took to finding where to fix is as follows:

  1. Found the error message NotFound: Specifier not found in cache: "https://jsr.io/@magurotuna/test-add/0.1.0/mod.ts", --cached-only is specified.
  2. Spotted where this message comes from:
    if (options.cacheSetting === "only") {
    throw new Deno.errors.NotFound(
    `Specifier not found in cache: "${specifier.toString()}", --cached-only is specified.`,
    );
    }
  3. Wondered why cacheSetting is set to only even though its default is use and I didn't specify it. Discovered that in deno_graph there is a place calling to load with CacheSetting::Only forcibly: https://github.com/denoland/deno_graph/blob/019728c7628d6b47222894c1356a3c9202d9204a/src/graph.rs#L3953
  4. Also, according to the CacheSetting::Only doc comment, the intended behavior of load method when target file is not found in the cache is to return Ok(None), not an error. This can be verified by looking at FetchCacher load impl in Deno CLI, where it attempts to fetch a requested file, and then if NotFound error is returned it is converted into Ok(None).

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had to look into this a bit. LGTM!

@dsherret dsherret merged commit e17687c into denoland:main Oct 28, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants