Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add dynamic imports in scripts tip #118

Merged
merged 14 commits into from
May 28, 2024
Merged

feat: add dynamic imports in scripts tip #118

merged 14 commits into from
May 28, 2024

Conversation

OliverSpeir
Copy link
Collaborator

@OliverSpeir OliverSpeir commented May 21, 2024

Closes #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?

Copy link

cloudflare-workers-and-pages bot commented May 21, 2024

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

Copy link

@JusticeMatthew JusticeMatthew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 7: ...this is great for a balance between performance and...

Line 19: ...allowing for manual optimization...

Line 64: ...Ensuring that modules somethings missing here critical for snappy INP....

Line 75: ...only when it is clicked you that could...


Dynamic imports also allow you to control the sequence of network requests. Instead of fetching all modules in parallel, which is the default behavior when using multiple static imports or separate module scripts, dynamic imports enable you to ensure that one module is downloaded first before initiating the fetch for the next module. This can be particularly useful when you have modules that are critical for immediate user interaction and others that are less critical or if you are supporting users with very low bandwidth.

## Example
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the examples below can be improved by 1:

  • Using real modules and using them (ie. must be basic to avoid distracting, you can use EC line markers)
  • Explain why using 1 example or another is better for those modules
    Overall being more practical I guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas for real world modules? I used confetti for the example repo but wasn't creative enough to come up with real real world examples lol

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean confetti is better than "..."!

@delucis
Copy link

delucis commented May 23, 2024

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!

@OliverSpeir
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

OliverSpeir commented May 23, 2024

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@florian-lefebvre
Copy link
Member

NWTWWHB!

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, we're almost there I think! Can you clean up the spacing a little bit? It's inconsistent across the tip

OliverSpeir and others added 2 commits May 27, 2024 01:48
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@OliverSpeir OliverSpeir merged commit 2fd0462 into main May 28, 2024
1 check passed
@OliverSpeir OliverSpeir deleted the dynamic-imports branch May 28, 2024 14:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic imports in scripts
5 participants