Add combinatorial basic module loaders in boa_interop #3785
Conversation
| /// This predicate will return an error if the specifier is relative but the referrer | ||
| /// does not have a path, or if the resolved path is outside `base`. | ||
| #[inline] | ||
| pub fn path_resolver(base: PathBuf) -> impl Fn(Option<&Path>, JsString) -> JsResult<JsString> { |
There was a problem hiding this comment.
@jedel1043 This function will fix the bug in the SimpleModuleLoader. I will move this code into the SimpleModuleLoader as a free function, and use that function here instead. It should make it easier.
I'll duplicate the tests as well.
These can be useful in general for creating chains of custom module loaders without much boilerplate.
|
@jedel1043 This is ready to review when you're ready. |
| // When resolving modules we need to clone the second loader. | ||
| second: Rc<Second>, |
There was a problem hiding this comment.
Thought: Hmmm, I don't really like this because we already wrap MergeModuleLoader in an Rc. Maybe it's time to commit to the Rc wrapper and change the receiver of the ModuleLoader methods to Rc<Self>.
There was a problem hiding this comment.
Not actionable from your side, I'm just documenting my thoughts.
There was a problem hiding this comment.
I think this would be solved rather elegantly if we could return a JsResult<Module> or similar from load_imported_module. But that's going to take a longer time and more refactoring than necessary.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3785 +/- ##
==========================================
+ Coverage 47.24% 50.26% +3.02%
==========================================
Files 476 461 -15
Lines 46892 45025 -1867
==========================================
+ Hits 22154 22634 +480
+ Misses 24738 22391 -2347 ☔ View full report in Codecov by Sentry. |
These can be useful in general for creating chains of custom module loaders without much boilerplate.