Get ImportProvider working with mutability and provided methods. #147

ac-z · 2024-06-28T17:30:24Z

This new branch has ImportProvider working with mutability for struct members, allowing for the dynamic importing functionality I initially described. Without the custom_import feature enabled, permissions checking remains same. Imports that use file or http/https are not able to be overridden by ImportProvider, but I don't think that's a huge loss.

Permissions checking in RustyLoader's resolve method occurs after the ImportProvider's (or deno's) resolve method is called. This means that if the user makes a custom specifier resolve to an http or https url while url_import is disabled, the resolution will fail as web imports are not allowed, even though the specifier doesn't contain http or https on the javascript side.

I also added two separate examples, which I think demonstrate why I think ImportProvider's import and resolve should be provided methods; they have separate use cases and should be able to be used conveniently without both methods implemented. The exact implementations provided by the trait may not be final.

EDIT: There's also a couple formatting anomalies. I will clean this up later today.

src/module_loader.rs

rscarson · 2024-06-28T18:29:04Z

src/module_loader.rs

+                                    let maybe_referrer = maybe_referrer.clone();
+                                    let requested_module_type = requested_module_type.clone();


You're cloning these twice
Not sure you need to clone at all? This block should be able to take ownership no?

When I remove those two lines, I got errors. Will get more info on this later.

Specifically, if I remove the first clones, I get "lifetime may not live long enough" and if I don't do the second clone, I get a "can't move out of captured variable" which occurs because Option<Url> doesn't have Copy. I left both clones in there with the intention of fixing it when I could understand what's going on there better.

src/module_loader.rs

Cargo.toml

src/module_loader.rs

examples/custom_import_schemes.rs

…import_provider

…error when feature not enabled)

ac-z · 2024-07-01T19:42:08Z

Sorry for the delay here. That test isn't written yet and a couple parts maybe could be a bit cleaner, but I think these changes are a bit more like what you were looking for. Feel free to suggest more changes.
Also, I don't know why maybe_referrer needs to be in an Rc in order to avoid the extra clone but the others are fine. Will investigate further.
The import method on the ImportProvider trait still has the same return as before, but since file and http/https are placed in the match statement before the custom behavior is reached, the provided implementation of that method only denies schemes that rustyscript doesn't already recognize. We'd have to do some weird shit in order to make the alternative work. If that method returns an option, it has to be handled inside the returned future, which means we'd need some way to route back to the logic for the other schemes, requiring a decent amount of rewriting.
Unless we want to execute the user's load method outside of the boxed local async block?

ac-z · 2024-07-02T04:56:27Z

The import method on the ImportProvider trait still has the same return as before, but since file and http/https are placed in the match statement before the custom behavior is reached, the provided implementation of that method only denies schemes that rustyscript doesn't already recognize. We'd have to do some weird shit in order to make the alternative work. If that method returns an option, it has to be handled inside the returned future, which means we'd need some way to route back to the logic for the other schemes, requiring a decent amount of rewriting. Unless we want to execute the user's load method outside of the boxed local async block?

Nvm figured it out, see commit.

ac-z · 2024-07-02T05:48:15Z

And that's the test done. Feedback is appreciated.

rscarson · 2024-07-04T20:06:03Z

I have not forgotten about this, btw
Just trying to get 0.5.0 working and out, and this can be 0.5.1 immediately after

ac-z · 2024-07-04T20:27:50Z

I have not forgotten about this, btw Just trying to get 0.5.0 working and out, and this can be 0.5.1 immediately after

Of course, don't worry about it. I will take care of any merge work that will be necessary.
If you have any more ideas about how to make this feature better before we reach that point, I will try them out.

…port_provider

ac-z · 2024-07-15T15:31:29Z

@rscarson There's a handful of "unused variable" and "variable is never read" warnings which only appear when the feature gate is disabled. Is it preferable to silence these with underscores or #[allow(unused)]?

rscarson · 2024-07-15T15:36:35Z

@rscarson There's a handful of "unused variable" and "variable is never read" warnings which only appear when the feature gate is disabled. Is it preferable to silence these with underscores or #[allow(dead_code)]?

Where are the warnings?

ac-z · 2024-07-15T15:44:20Z

Where are the warnings?

warning: unused variable: `kind`
   --> src/module_loader.rs:155:9
    |
155 |         kind: deno_core::ResolutionKind,
    |         ^^^^ help: if this is intentional, prefix it with an underscore: `_kind`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `maybe_referrer`
   --> src/module_loader.rs:210:9
    |
210 |         maybe_referrer: Option<&ModuleSpecifier>,
    |         ^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_maybe_referrer`

warning: unused variable: `is_dyn_import`
   --> src/module_loader.rs:211:9
    |
211 |         is_dyn_import: bool,
    |         ^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_is_dyn_import`

warning: unused variable: `requested_module_type`
   --> src/module_loader.rs:212:9
    |
212 |         requested_module_type: deno_core::RequestedModuleType,
    |         ^^^^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_requested_module_type`

warning: field `import_provider` is never read
  --> src/module_loader.rs:48:5
   |
46 | struct InnerRustyLoader {
   |        ---------------- field in this struct
47 |     cache_provider: Rc<Option<Box<dyn ModuleCacheProvider>>>,
48 |     import_provider: Rc<Option<RefCell<Box<dyn ImportProvider>>>>,
   |     ^^^^^^^^^^^^^^^
   |
   = note: `InnerRustyLoader` has a derived impl for the trait `Clone`, but this is intentionally ignored during dead code analysis
   = note: `#[warn(dead_code)]` on by default

warning: `rustyscript` (lib) generated 5 warnings

It is mostly regarding parameters that only get used inside feature-gated bits of their scope.
Also import_provider in InnerRustyLoader is just never checked by anything if the feature is off.

rscarson · 2024-07-16T12:33:05Z

Ah I see, then yeah probably safe to ignore
But that field should probably be feature-gated too, as should the inclusion of whatever file ImportProvider lives in

… block.

…der.

…port_provider

ac-z · 2024-07-19T03:24:31Z

Whoops, messed up the merge on the test section. Will get that in a bit.

ac-z · 2024-07-19T04:39:01Z

@rscarson I made sure to more thoroughly feature-gate every instance where ImportProvider is used, even in function parameters. I've just been adding to module_loader.rs thus far, so gating the whole file wouldn't make much sense at the current moment, but if you'd prefer I can split this feature off into its own file. Now that everything's working as expected, I am aiming to finalize this PR to be merged. I can also try to improve the doc comments and simplify the examples if needed.

rscarson · 2024-07-19T13:34:32Z

Cargo.toml

+
+[[example]]
+name = "async_javascript"
+required-features = ["web_stub"]


Ah good catch on this

rscarson · 2024-07-19T13:35:42Z

src/inner_runtime.rs

@@ -131,7 +137,11 @@ pub struct InnerRuntime {
 }
 impl InnerRuntime {
    pub fn new(options: RuntimeOptions) -> Result<Self, Error> {
-        let loader = Rc::new(RustyLoader::new(options.module_cache));
+        let loader = Rc::new(RustyLoader::new(


I might move these into a LoaderOptions or something eventually but that's not public facing so not a big deal

src/module_loader.rs

…o.toml

rscarson · 2024-07-20T20:15:40Z

LGTM! I'll merge this in Monday! Thanks for all your hard work!

…port_provider

rscarson · 2024-07-23T18:00:04Z

I had to make a few changes, but the new trait is now integrated and working
I also modified it in a way that feature-gating is no longer required

utyfua · 2024-10-21T17:22:28Z

src/module_loader.rs

@@ -126,10 +158,21 @@ impl ModuleLoader for RustyLoader {
        &self,
        specifier: &str,
        referrer: &str,
-        _kind: deno_core::ResolutionKind,
+        kind: deno_core::ResolutionKind,
    ) -> Result<ModuleSpecifier, anyhow::Error> {
        // Resolve the module specifier to an absolute URL
        let url = deno_core::resolve_import(specifier, referrer)?;


Is it possible to create a custom url resolver for a module specifier?

I want to make imports look like ... from 'my-module' to be compatible with Node.js. However, I'm encountering an error from here:

Relative import path "my-module" not prefixed with / or ./ or ../ from "file:///<...>/index.js"

WIP Get ImportProvider working with mutability and provided methods.

df89881

rscarson reviewed Jun 28, 2024

View reviewed changes

ac-z added 6 commits July 1, 2024 08:48

Merge branch 'master' of gh:rscarson/rustyscript into feature/custom_…

c5c6bfb

…import_provider

WIP Gate the pub use statement instead of the trait itself. (Fixes …

531cdde

…error when feature not enabled)

WIP Change ImportProvider's resolve method to return an Option.

8db9164

WIP Avoid unneccessary clone. Use Rc instead?

8b0268c

WIP Replace match statement with .map() to reduce clutter.

4c2f9f7

WIP Change ImportProvider's feature flag name.

caa643f

WIP Change ImportProvider's import method to return option.

305f3f4

ac-z added 2 commits July 1, 2024 22:38

WIP Add test_import_provider to existing module loader tests.

0ed9a1a

WIP add tentative doc comments to ImportProvider.

c96f366

WIP Fix feature flags on ImportProvider test.

df2c58d

ac-z added 7 commits July 14, 2024 15:33

WIP Improve comments in custom import example.

7da17d4

WIP ImportProvider test formatting.

c01fa89

WIP Add clarifying comment to import resolution example.

1b9c44b

Merge remote-tracking branch 'upstream/master' into feature/custom_im…

9af2362

…port_provider

WIP Format custom import example.

7d0651d

WIP FIx incorrect function call in snapshot builder code.

1609a01

WIP Fix formatting.

fdf20d3

ac-z added 2 commits July 18, 2024 18:55

WIP Feature-gate ImportProvider struct member & use statement in test…

9adf4a2

… block.

WIP Finish adding feature gates to internal references to ImportProvi…

dcb7963

…der.

ac-z added 2 commits July 18, 2024 19:29

WIP Missed one.

2fc1907

Merge remote-tracking branch 'upstream/master' into feature/custom_im…

e0b2fea

…port_provider

WIP Fix duplicated lines from merge.

8b32539

rscarson reviewed Jul 19, 2024

View reviewed changes

Cargo.toml

[[example]]

name = "async_javascript"

required-features = ["web_stub"]

Copy link

Owner

rscarson Jul 19, 2024

Choose a reason for hiding this comment

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

Ah good catch on this

rscarson reviewed Jul 19, 2024

View reviewed changes

src/module_loader.rs Outdated Show resolved Hide resolved

ac-z added 2 commits July 19, 2024 19:42

WIP Remove unnecessary anyhow::Ok

7e239e5

WIP Correct comment regarding optional module loader features in Carg…

29da868

…o.toml

ac-z changed the title ~~WIP Get ImportProvider working with mutability and provided methods.~~ Get ImportProvider working with mutability and provided methods. Jul 21, 2024

ac-z added 4 commits July 21, 2024 07:55

Add import_provider feature to readme.md

0a801a2

Add import_provider feature to crate docs

fdb9d8f

Make daggers superscript in doc comment for readability

ce97a85

Merge remote-tracking branch 'upstream/master' into feature/custom_im…

a2e11dd

…port_provider

rscarson merged commit 019acf8 into rscarson:master Jul 23, 2024
4 checks passed

utyfua reviewed Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get ImportProvider working with mutability and provided methods. #147

Get ImportProvider working with mutability and provided methods. #147

ac-z commented Jun 28, 2024 •

edited

Loading

rscarson Jun 28, 2024

ac-z Jun 28, 2024 •

edited

Loading

ac-z Jun 29, 2024 •

edited

Loading

ac-z commented Jul 1, 2024 •

edited

Loading

ac-z commented Jul 2, 2024

ac-z commented Jul 2, 2024

rscarson commented Jul 4, 2024

ac-z commented Jul 4, 2024 •

edited

Loading

ac-z commented Jul 15, 2024 •

edited

Loading

rscarson commented Jul 15, 2024 •

edited

Loading

ac-z commented Jul 15, 2024 •

edited

Loading

rscarson commented Jul 16, 2024

ac-z commented Jul 19, 2024

ac-z commented Jul 19, 2024 •

edited

Loading

rscarson Jul 19, 2024

rscarson Jul 19, 2024

rscarson commented Jul 20, 2024

rscarson commented Jul 23, 2024

utyfua Oct 21, 2024

		let maybe_referrer = maybe_referrer.clone();
		let requested_module_type = requested_module_type.clone();

Get ImportProvider working with mutability and provided methods. #147

Get ImportProvider working with mutability and provided methods. #147

Conversation

ac-z commented Jun 28, 2024 • edited Loading

rscarson Jun 28, 2024

Choose a reason for hiding this comment

ac-z Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

ac-z Jun 29, 2024 • edited Loading

Choose a reason for hiding this comment

ac-z commented Jul 1, 2024 • edited Loading

ac-z commented Jul 2, 2024

ac-z commented Jul 2, 2024

rscarson commented Jul 4, 2024

ac-z commented Jul 4, 2024 • edited Loading

ac-z commented Jul 15, 2024 • edited Loading

rscarson commented Jul 15, 2024 • edited Loading

ac-z commented Jul 15, 2024 • edited Loading

rscarson commented Jul 16, 2024

ac-z commented Jul 19, 2024

ac-z commented Jul 19, 2024 • edited Loading

rscarson Jul 19, 2024

Choose a reason for hiding this comment

rscarson Jul 19, 2024

Choose a reason for hiding this comment

rscarson commented Jul 20, 2024

rscarson commented Jul 23, 2024

utyfua Oct 21, 2024

Choose a reason for hiding this comment

ac-z commented Jun 28, 2024 •

edited

Loading

ac-z Jun 28, 2024 •

edited

Loading

ac-z Jun 29, 2024 •

edited

Loading

ac-z commented Jul 1, 2024 •

edited

Loading

ac-z commented Jul 4, 2024 •

edited

Loading

ac-z commented Jul 15, 2024 •

edited

Loading

rscarson commented Jul 15, 2024 •

edited

Loading

ac-z commented Jul 15, 2024 •

edited

Loading

ac-z commented Jul 19, 2024 •

edited

Loading