Skip to content

Conversation

wbk
Copy link
Contributor

@wbk wbk commented May 17, 2025

The libffi dependency introduced (by way of deno_ffi) by #366 has caused master to break for me on macos; I decided to take a stab at a refactor to support 2.3.3 instead.

@wbk
Copy link
Contributor Author

wbk commented May 20, 2025

I realize there are a lot of moving parts here, but there's so much codependence between all of deno's deps, I'm not certain I could have "done less". Are PRs introducing this amount of change welcome (and is there any better way I could structure this request?)

@rscarson
Copy link
Owner

Sorry! I have a newborn at home so I haven't had time to take a look yet!

I've approved the checks

ext.esm_files = ::std::borrow::Cow::Borrowed(&[]);
ext.esm_entry_point = ::std::option::Option::None;
}
// Clears the js and esm files for warmup to avoid reloading them
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm skeptical this is still necessary post-denoland/deno_core@aa6f360 (In fact, I think this entire trait can probably go away in favor of calling Extension::init directly)

let referrer_specifier = referrer
.to_module_specifier(&self.cwd)
.map_err(|e| JsErrorBox::from_err(to_io_err(e)))?;
let referrer_specifier = if deno_core::specifier_has_uri_scheme(referrer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed to fix one of the module loader unit tests, deno seems to be storing module paths internally as file:/// urls rather than preserving the relative paths from source

@@ -31,6 +32,7 @@ use sys_traits::impls::RealSys;
use super::cjs_translator::{NodeCodeTranslator, RustyCjsCodeAnalyzer};

const NODE_MODULES_DIR: &str = "node_modules";
const TYPESCRIPT_VERSION: &str = "5.8.3";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was lifted from https://github.com/denoland/deno/blob/395878c5fb72d87a44f7c88ce9f391acc31feab3/cli/lib/version.rs

It might need to be kept in sync somehow?

@wbk
Copy link
Contributor Author

wbk commented May 20, 2025

No worries and congrats! I left a few callouts as notes

@getong
Copy link

getong commented Aug 15, 2025

this pr is great, would somebody take a look at it and merge it?

@rscarson rscarson merged commit 1821240 into rscarson:master Aug 15, 2025
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