astro-tips.dev icon indicating copy to clipboard operation
astro-tips.dev copied to clipboard

feat: add dynamic imports in scripts tip

Open OliverSpeir opened this issue 1 year ago • 4 comments

Closes https://github.com/astrolicious/astro-tips.dev/issues/93

Ready for review, this is a really information heavy tip so any writing style reviews would be helpful.

Basically... does this make sense? does it sufficiently explain things?

OliverSpeir avatar May 21 '24 08:05 OliverSpeir

Deploying astro-tips with  Cloudflare Pages  Cloudflare Pages

Latest commit: 996362d
Status: ✅  Deploy successful!
Preview URL: https://63c7f7c1.astro-tips-blf.pages.dev
Branch Preview URL: https://dynamic-imports.astro-tips-blf.pages.dev

View logs

Nice work @OliverSpeir!

Some quick thoughts:

  • with Astro, script tags are bundled into a single JS file, so in fact static imports don't usually cause any additional network requests. You mention "parallel" requests, which is true for imports in a type="module" that has been written manually. In Astro, it's probably more accurate to say dynamic imports "defer" what would otherwise be loaded up front.

  • One benefit of bundling is being able to take more advantage of compression (i.e. gzip or brotli), because as a rule, the larger the file, the better it compresses. That means prematurely splitting a bundle with dynamic imports can lead to bigger download sizes potentially. Splitting is definitely best applied when some key logic needs prioritising (or like in some of your examples where the code may be never needed at all).

  • I'm not super familiar with the Astro Tips style yet, but this felt like quite a bit of preamble about Web Vitals etc. before getting to the core tip. It's all super well written, so if that's the style, it's great! But could also be something to consider whether you want to communicate earlier in the page WHAT exactly the technique is. (You have to get all the way down to the Example heading to actually see the import () syntax for example.

Hope that's a bit helpful, but honestly it's already looking great!

delucis avatar May 23 '24 10:05 delucis

@delucis I just removed the core web vitals stuff you're right no need to yap so much its meant to be an easily digestable tip not a monologue

and thank you for fact checking I lowkey forgot about the consequences of astro bringing everything into a single file

OliverSpeir avatar May 23 '24 11:05 OliverSpeir

Should probably work in here how this can affect total blocking time ( with the client visible or rare button click examples )

edit: actually I've come to conclusion it's better to just explain this very simply and let the users draw any conclusions

OliverSpeir avatar May 23 '24 12:05 OliverSpeir

Ok upon further inspection I think astro hoists all into on main script but it does in create separate .js files for all code that is imported in those script tags (it dedupes them obviously). So it is accurate to be able to granularly control network requests, because the main hoisted .js file will by default have static imports which are all fetched in parallel

OliverSpeir avatar May 26 '24 19:05 OliverSpeir

@florian-lefebvre I've changed the imports from "..." to the fake names, I went to add the confetti in there but I felt it detracted from the clarity of the tip because having it called criticalModule and lessCriticalModule I think makes it easier to understand

Is this ok with you? I know it's still not ideal

<script>
  import criticalModule from 'criticalModule';
  criticalModule();

  const lessCriticalModule = await import('lessCriticalModule');
  lessCriticalModule();
</script>

OliverSpeir avatar May 26 '24 19:05 OliverSpeir

NWTWWHB!

florian-lefebvre avatar May 27 '24 05:05 florian-lefebvre