Provide the raw specifier for the module fallback service #2131
Conversation
|
I took a stab at testing this using a local build from this PR and I can confirm that the I modified @dario-piotrowicz's app to test this. You can see my changes here: https://github.com/dario-piotrowicz/miniflare-ModuleFallbackService-demo/compare/main...IgorMinar:miniflare-ModuleFallbackService-demo:workerd-pr-2131-test?expand=1 Overall, while I succeeded in getting the fallback service to work, I had to hack around a lot and I don't have confidence that this will fly in the real world, without further changes in workerd. Specifically workerd's inability to support bare specifiers and specifiers ending with Btw I do find Interestingly, I found that workerd resolved specifier is still useful to detect when a redirect is needed, which implies that we actually need both the workerd-resolved specifier as well as the raw specifier. This surprised me, but it's likely a good news for us as we don't need to deprecate and remove the original Let's discuss this a bit before this change is merged. side note: I had to employ 301 redirects to get everything to work, but I think that is expected, as this is exactly why the fallback service supports the redirects at all. |
| @@ -2753,6 +2754,11 @@ kj::Own<Server::Service> Server::makeWorker(kj::StringPtr name, config::Worker:: | |||
| kj::Url::QueryParam { kj::str("referrer"), kj::mv(ref) } | |||
| ); | |||
| } | |||
| KJ_IF_SOME(raw, rawSpecifier) { | |||
| url.query.add( | |||
| kj::Url::QueryParam { kj::str("raw"), kj::str(raw) } | |||
There was a problem hiding this comment.
| kj::Url::QueryParam { kj::str("raw"), kj::str(raw) } | |
| kj::Url::QueryParam { kj::str("rawSpecifier"), kj::str(raw) } |
remove a bunch of hacks
more clean up and splitting up the namespace into _vite_bare_ and _vite_externals_
update docs
There was a problem hiding this comment.
Dario and I hacked on the module fallback service with this PR and we were able to get it all working.
We came across some new bugs and ultimately failed to use : prefixed modules with the fallback service, but this is not a show stopper for us, and I'm not quite sure if the issue is worth fixing before your module registry lands. (the issue is that internally in workerd node_modules:foo when imported from /my-app/index.js is internally resolved to /my-app/node_modules:foo because the current specifier resolver doesn't treat node_modules:foo as an absolute specifier. I do recommend that you try to add this as a test to your module registry PR so that we avoid reintroducing this issue in your rewrite.
But ultimately the PR is good enough for us, and I think we should merge your PR and release it, so that we can do more through testing of this code outside of our small demos.
Our test code can be seen here: https://github.com/dario-piotrowicz/miniflare-ModuleFallbackService-demo/pull/1/files
|
btw I can't give you review on the c++ code as I'm not familiar with the code base, so my approval is simply just stating that from the external point of view the code does what we asked for it to do. It would be good to have someone from the runtime team to confirm the correctness of the implementation. |
|
@fhanau @dom96 @garrettgu10 ... would like to get this landed soon to unblock @IgorMinar's team. Can I ask one of y'all to do a quick review on this? |
Fixes: #2112