superfly / fly.rs

Fly Edge Application runtime.
https://fly.io
MIT License
39 stars 7 forks source link

modular module resolver system. #20

Closed afinch7 closed 5 years ago

afinch7 commented 5 years ago

A Modular system for adding additional module resolvers into a runtime.

Should make it easier to implement systems involving "virtual modules".

I've mostly hooked up a basic implementation of https://github.com/superfly/fly.rs/issues/16 Sadly it doesn't quite work right now, but it's very close.

Things that still need to fixing:

Some ideas/feedback would be much appreciated.

jeromegn commented 5 years ago

I'm impressed you were able to get this running at all! We've not documented the process thoroughly. Nice job.

I have a few general comments:

afinch7 commented 5 years ago

I'm impressed you were able to get this running at all!

I'm kinda impressed myself since I had zero experience with rust prior. I really didn't have much interest in rust prior to this, but I can now see why it was a much better choice than c++ for this project.

Runtime::new should probably start accepting a configuration struct or use a builder pattern given the number of options it now has. This is not required for this PR to be accepted though.

I figure adding this into the "Settings" struct should work fine. I will likely do that at some point before this gets merged.

It sounds like we should support .json importing by default. Like import * as secrets from "./secrets.json". This would remove the need for virtual modules for your specific use case.

The way I have it setup with the secrets.json file is simply a demo. My end goal is to be able to import private api or domain code from a "virtual repository" of sorts. This would make the whole platform alot more powerful as a micoservice framework.

Just picture something like:

This scheme could quite easily create an entirely new workflow that hasn't really been seen yet.

Hopefully at some point with a little more effort a system like this can be fully integrated into a development environment including types, source code, jsdoc info, etc.

Without going too deep my end goal with this project is to design a better platform for developing complex systems and services, and finally modernize some of the aspects of microservice development mainly the dependency systems.

Also I just located the file exists check that was causing ts language services to complain I will see if I can get that worked out along with a basic json loader, and get this pr finished up.

michaeldwan commented 5 years ago

@afinch7 this looks really good. We want resolvers for json, node_modules, and urls, and this PR is a great start! I'm going to take a closer look in the next few days then let you know how we can move forward.

In the meantime, one thing to think about is that the typescript compiler increases the runtime heap by at least 30mb which won't work well in a multi-tenant environment. In production, we run optimized javascript code that won't have dev-tools.js available, so we can't rely on typescript's module resolution alone for secrets. I'm not sure how dynamic your use case is, but I think we can accomplish much of the same thing by using dynamic resolution in dev and generating code to read injected secrets in prod using rollup+typescript in the dev tools, which will soon be used to create optimized builds entirely within the fly.rs cli.

I'm on our channel if you'd like to talk through any of this... I'd love to hear more about what you're building!

afinch7 commented 5 years ago

I would say my use case would be pretty dynamic. It didn't really occur to me that you aren't already using ES6 modules for execution until after I had dug through everything. I now realize that the only good way to do this is to implement ES6 module execution and stack up a resolver system and a typescript compiler(loader) on top. I figure the new import process should look something like this:

  1. An import resolver loads the source code and presents it to a loader controller/manager.
  2. A loader is chosen based on media type and is used to "load"(compile to js or wasm if needed) source code.
  3. The resulting code will be put into a ES6 module that will be returned by the import callback.

This will allow for very dynamic resolution if needed, and solutions like rollup should still work fine too.

I don't see any reason why this should break the current system with dev-tools, but I might be wrong about that. It should be pretty easy to maintain the current resolver api and just add some other layers underneath; basically just call the import resolver and return the resulting source code directly back to dev-tools.

This should also give an added bonus of making wasm support an easy bolt on addon, so fly app in rust, go, or any other language that has a wasm compiler out there. I see a lot of value in embracing wasm, multi-threading already exists in v8's wasm apis last I heard.

michaeldwan commented 5 years ago

This is looking great @afinch7!

afinch7 commented 5 years ago

This is pretty much ready to go. It's still missing node_module resolution, but I can add that later.

jeromegn commented 5 years ago

Stray comment: @afinch7 can you update your flatc to at least 1.10. msg_generated.rs does not need as many lifetime specified anymore.

jeromegn commented 5 years ago

One thing we're concerned about is production. Right now, we don't bundle people's app with this project, we still use the node project. Meaning we get fully static JS bundles from it and just evaluate that in this project (without the dev-tools). I assume this will work fine even with your changes. Maybe we should pass a Some(vec![]) instead of a None at runtime creation in the distributed-fly crate.

afinch7 commented 5 years ago

One thing we're concerned about is production. Right now, we don't bundle people's app with this project, we still use the node project. Meaning we get fully static JS bundles from it and just evaluate that in this project (without the dev-tools). I assume this will work fine even with your changes. Maybe we should pass a Some(vec![]) instead of a None at runtime creation in the distributed-fly crate.

Now that I think about it this may in fact be a security issue. I will make that change. I don't think that it should cause any problems with fully bundled code though.

afinch7 commented 5 years ago

I wonder how this will affect compilation for bundles. Does it still create fully static bundles (all concatenated, not relying on any ES module) or does it now import modules at runtime? It looks like this is still using AMD for modules, unless I missed that change. I'm not sure how AMD and ES modules can interop.

There may be something I missed, but I don't see any reason why my changes so far should. I didn't move execution over to ES modules yet. I plan to move the execution over in the future, but the infrastructure isn't there yet for that. When I do make that change though, the only thing that I'm not completely sure about is how v8 handles global in respect to modules and contexts. I'm pretty sure, based on what I've experimented with in the past, that even after that change bundled code should just run like a module with no dependencies.

jeromegn commented 5 years ago

Oh it’s just resoVLe vs resoLVe :)

On Wed, Jan 16, 2019 at 2:07 PM andy finch notifications@github.com wrote:

@afinch7 commented on this pull request.

In src/module_resolver.rs https://github.com/superfly/fly.rs/pull/20#discussion_r248413125:

    • Resolves a module specifier and returns a "strategy" for loading the module to ES6 or WASM code.
  • */ +pub trait ModuleResolver: Send {
  • fn resolve_module(
  • &self,
  • module_specifier: Url,
  • referer_info: Option,
  • ) -> FlyResult;
  • fn get_protocol(&self) -> String; +}
  • +/**

    • This trait is a used as the "front door" of the dynamic module resolution system.
  • */ +pub trait ModuleResolverManager: Send {
  • fn resovle_module(&self, specifier: String, referer_info: Option) -> FlyResult;

Do you mind being more specific I don't see this one. I do think that renaming it might be a good idea though to avoid confusion between ModuleResolverManager::resolve_module and ModuleResolver::resolve_module, as they do very different things.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/superfly/fly.rs/pull/20#discussion_r248413125, or mute the thread https://github.com/notifications/unsubscribe-auth/AACpPerUEeiV9ByLbUAegINRt1BNkQSaks5vD3h3gaJpZM4Zkxun .

afinch7 commented 5 years ago

That should fix all those issues you can give a second pass whenever you are ready.

michaeldwan commented 5 years ago

@afinch7 I'm giving this a look now. There's a number of build warnings and some outdated generated code. I'm going to update that stuff in your branch to make reviewing easier. What version of flatc are you using?

michaeldwan commented 5 years ago

I went ahead and fixed the compiler warnings. The remaining suggestions can be fixed in future PRs. Awesome work!