Get ImportProvider working with mutability and provided methods.

[ac-z]

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.

[ac-z]

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]

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]

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

[rscarson]

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]

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.

[ac-z]

@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]

@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]

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>>, 48	import_provider: Rc<Option<RefCell<Box>>>,	^^^^^^^^^^^^^^^

= 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]

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

[ac-z]

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

[ac-z]

@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]

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

[rscarson]

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