I ended up reporting https://github.com/
I ended up reporting https://github.com/cloudflare/next-on-pages/issues/722, as I think if solvable it could help mid/large projects move successfully to Cloudflare. I wasn't able to trace the issue, as I do see the routes being lazy loaded perfectly (e.g.
await import(item.entrypoint)) but can't really see what wrangler is doing internally (tried --no-build with an upcoming fix and also debugging internally and couldn't really see anything wrong, route-specific code seems to be only in additionalModules, etc.).
If anyone's have any ideas or things worth trying, I'm happy to try it out! Thanks!25 Replies
Hey @juanferreras. thanks a lot for the issue 🙂
I've replied there
anyways the thing worth trying now, as I mentioned there, I think could be seeing if this is a general Cloudflare issue or a next-on-pages specific one (since I think/feel that there's a good chance that it's not a next-on-pages issue)
so that we can then better understand the thing and hopefully have a more clear direction to investigate afterwords 🙂
hey @Dario thanks for the response! that's actually a great shout I was hesitating with - but I was looking into some public comms on
workers-sdk and seemed like next-on-pages is by far the best example of the lazy loading/dynamic importing pattern haha
I'd love to create the new repro, would you suggest just something like https://github.com/cloudflare/workers-sdk/tree/main/fixtures/pages-workerjs-directory/public but simplified to the bare minimum? LMK if you had any specifics in mind and I'll do it so you don't have toyeah I was thinking something like that
if you want to have an already almost minimum boilerplace app you could use this 🙂
https://github.com/dario-piotrowicz/cf-pages-_worker.js-dir-example/tree/main
GitHub
GitHub - dario-piotrowicz/cf-pages-_worker.js-dir-example
Contribute to dario-piotrowicz/cf-pages-_worker.js-dir-example development by creating an account on GitHub.
I'd basically take that, strip the assets logic, even strip the html logic there (but that's not necessary) and simply try to play with dynamic imports 🙂
similarly on how you did in your repro and again use this somewhere https://github.com/juanferreras/next-cold-start/blob/main/src/components/large-data.js 🙂
GitHub
next-cold-start/src/components/large-data.js at main · juanferreras...
Contribute to juanferreras/next-cold-start development by creating an account on GitHub.
perfect I'll give this a try and share a repro if I can replicate it in the next few days – many thanks Dario!
no problem! thanks so much for all your effor on this! ❤️
hey @Dario sorry for the quick follow up but I was just too curious haha.
You were 100% right, it can be easily replicated without next-on-pages https://github.com/juanferreras/dynamic-import-cold-start/blob/main/public/_worker.js/index.ts. Would you like me to close the issue and open it somewhere else? Or do you want me to post the new reproduction on the original issue?
- with large.mjs https://www.webpagetest.org/result/240326_BiDcXB_EHT/?test=240326_BiDcXB_EHT&medianMetric=TTFB
- without large.mjs https://www.webpagetest.org/result/240326_BiDcT2_EHV/?test=240326_BiDcT2_EHV&medianMetric=TTFB
Hey @juanferreras. no need to apologize, that's amazing that you looked into it so quickly, thanks so much for that ❤️
I'm not too sure what to think about the fact that it is not next-on-pages related... although it means that we're not doing anything wrong there I'm afraid it can also mean that this is an inherit issue in the Cloudflare network that we can't do too much about (as I mentioned earlier 🥲)
If you could, could you please open a new issue with the new repro in the workers-sdk repo? (https://github.com/cloudflare/workers-sdk/issues/new?assignees=&labels=bug&projects=&template=bug-template.yaml&title=%F0%9F%90%9B+BUG%3A)
It's likely not the most appropriate place for such an issue, but it is more appropriate than next-on-pages, and we can use it to track this issue I guess 🙂
GitHub
Build software better, together
GitHub is where people build software. More than 100 million people use GitHub to discover, fork, and contribute to over 420 million projects.
ah! actually I'm so sorry I just tought of an extra step we can take this first if you'd still have an extra bit of time!
could you maybe convert your latest minimal reproduction in a standard worker?
just to also rule out any pages related issues 🙂
what I'm thinking of is just, moving
_worker.js directory at the root, remove the public directory, add an appropriate wrangler.toml and it should be good to go 🙂yeah absolutely! do you have any similar very-minimal template I can use as inspiration/guidance? your last comment is very helpful
This https://github.com/cloudflare/workerd/pull/1553 sounds related - I wonder if James S & team are actually aware of pitfalls like these in the current implementation but have a future plan for improvements. In any case, would you rather I submit an issue on
workers-sdk or workerd?yeah absolutely!thank you! ❤️ 🙏
do you have any similar very-minimal template I can use as inspiration/guidance? your last comment is very helpfulI'm glad it was helpful! 😄 Sure, here you are 🙂 : https://github.com/dario-piotrowicz/cf-worker-with-dynamic-imports-example
GitHub
GitHub - dario-piotrowicz/cf-worker-with-dynamic-imports-example
Contribute to dario-piotrowicz/cf-worker-with-dynamic-imports-example development by creating an account on GitHub.
This https://github.com/cloudflare/workerd/pull/1553 sounds relatedI'm not complerely sure it is related (it might be 🤷♂️)
I wonder if James S & team are actually aware of pitfalls like these in the current implementation but have a future plan for improvements.🤷♂️ (James is also off this week so I can't check with them 😅)
In any case, would you rather I submit an issue on workers-sdk or workerd?I think both make sense, but I'd probably go with
workers-sdk as it is more general, but workerd can be a good fit too, whatever you prefer 🙂
(but before posting to workerd maybe you should check if the issue happens locally as well, if it does not then probably I'd not open the issue in workerd as that would indicate that it's not really a workerd problem I think 🤔)GitHub
Initial implementation of new jsg module registry by jasnell · Pull...
RM-17318
First step of the new module registry implementation (the jsg only bits). Expect several more follow up PRs.
This PR provides the initial core of the new module registry implementation. It...
excellent, that's very insightful - I'll adapt it to a simple worker (instead of pages), try to see if locally I can replicate (cold starts weren't really a thing I was able to see locally), and most likely report the issue on workers-sdk (and close the next-on-pages). Thanks Dario!
my pleasure! and thank you too very much! ❤️
anyways yeah please keep me posted to that I'll look into things/bother the right people for it 😁👍
Hey Dario! As a final update on this - I've just closed the original issue on next-on-pages and put the new repros with the new findings in https://github.com/cloudflare/workers-sdk/issues/5415 so we can continue discussing/tracking over there. Thanks once again!
GitHub
🐛 BUG: Workers TTFB slows down based on unused dynamic imports (add...
Which Cloudflare product(s) does this pertain to? Pages, Workers Runtime, Wrangler core What version(s) of the tool(s) are you using? wrangler 3.38.0 What version of Node are you using? 18.18.2 Wha...
thanks @juanferreras.! 🙏
by the way, I was just chatting with a colleague regarding this and it looks like the workerd module registry should indeed help with the issue! 😅
(so you were right! 🙈)
as it seems like that would avoid eagerly parsing and compiling the code
the workerd PR can take a bit to get merged... so we'll have to keep an eye on things and see when it lands if it fixes (or at least improves) the issue
haha I can't really read c++ - but I just know jasnell is at least one step ahead of the rest of us 😂
for sure! it was interesting spotting differences between worker and pages - feels like that might actually be something in workers-sdk/wrangler if I didn't screw something up
if anything my perspective of all of this is: (a) 10 MB gzipped is a LOT of code (these experiments are only reaching 30% of that), (b) dynamic imports are fantastic, and (c) when ever we can mitigate (even if partially) their impact, pretty much any very-large/enterprise-y app should still fit in workers without hiccups
ah! yeah sorry I just now read the issue's description properly! interesting that the impact is different! 😯
hey @Dario, hope you've been great! I'm sorry for surfacing an old thread and directly tagging you, but am I right to believe from your comment in https://github.com/sveltejs/kit/issues/2963#issuecomment-2041512990, that you were able to confirm that:
A. the more additional modules code, the worse TTFB becomes due to the current module registry does indeed parse and compile (but not evaluate) all additional modules? This will be fixed in the new jsg registry but ETA for that is likely N months away
B. we aren't really sure of the differences between Workers/Pages yet / nothing comes to mind as things worth trying for to make those up, nor anything is immediately wrong on my test conditions
--
if that's right - would you mind (a) if I comment on the github issue with the above OR (b) you leaving a comment OR (c) internally asking who ever is more appropriate/owning the new registry if they wouldn't mind adding a quick comment explaining? This is more so anyone who suscribed/liked the issue can be also aware/updated.
thanks as always 🙏
Hi @juanferreras. no issue at all, thanks for the ping 🙂
Regarding your two points:
A. No, I spoke with the team and the module registry should definitely make it better but I do not know if it would completely fix your issue, that's not sure yet, we'll have to see (it is a bunch of work but if you want, when I have some extra time, I can try to build the workerd new code locally and do some benchmarking aginst your repro to see the impact locally (I can't remember, did you also see an impact of this locally? or only during deployment? 😕))
B. yeah, I didn't yet look that much into it, and I'm not sure what might be wrong, but your test conditions seem valid to me
Regarding the latter points, I don't mind either, please feel free to leave a comment or I can do it, whatever you prefer 🙂
also I can ask the creator of the new registry PR, but as I said it might or solve the issue 🤷♂️
but also in the svelteKit issue, I feel like that might be an issue unrelated to yours and that they might be including more modules than necessary in their output dir, I'd like to investigate that first 🙂 (I was planning to have a look into that this weekend)
A. Got it, that makes sense. Locally I haven't been able to see the issue, the workerd team might have tools to do local benchmarks but all the ones I know were not really signaling the problem (I'm unaware of whether cold-starts are even a thing that can be easily replicated locally). Over the network it's a lot easier (e.g. no localhost differences in case there's any, no computer resources differences, etc.). So whilst I really appreciate your suggestion, unless there's any way to try the runtime deployed and specially noting that's meaningful LOE, let's just wait for it to land somewhere we can benchmark it from, eventually.
B. Good, that's good to hear!
The best I think would be if the PR author has any thoughts, and I can sit tight until there's any compat flag or any way to run that runtime deployed at which point I can re-ran all the benchmarks and see if that makes a meaningful difference. Of course, being respectul of everyone's time and privacy as well.
--
Totally, I was just extrapolating from your comment on the svelteKit issue - it does not seem related
mh.... the fact that this is not replicable locally suggests me that the registry changes might not actually help.... 😓 (unless for whatever reason the modules' parsing and compilation might be more onerous in production 🤷♂️) hopefully I'm wrong, I'll try to investigate this a bit and ask the PR's author for his opinion 🙂
I'll let you know what they say 🙂👍
thank you so much, Dario!