Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat(cloudflare-pages, cloudflare-module): enable code splitting by default #1905

Merged
merged 4 commits into from
Nov 12, 2023

Conversation

dario-piotrowicz
Copy link
Contributor

@dario-piotrowicz dario-piotrowicz commented Nov 11, 2023

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

remove the NITRO_EXP_CLOUDFLARE_DYNAMIC_IMPORTS flag (added in #1172) used to enable code splitting with lazy loading for the cloudflare-pages and cloudflare_module presets and enable such behavior by default since that is not stable and working with up-to-date versions of wrangler

For the stability of this feature please have a look at these reproductions: https://github.com/dario-piotrowicz/nitro-nuxt-lazy-loading-test/tree/main

They show that things do work fine (both locally and in production) when using lazy loading πŸ’ͺ

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

remove the NITRO_EXP_CLOUDFLARE_DYNAMIC_IMPORTS flag used to enable code
splitting with lazy loading for the cloudflare-pages and
cloudflare_module presets and enable such behavior by default since that
is not stable and working with up-to-date versions of wrangler
Copy link
Contributor

nuxt-studio bot commented Nov 11, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
nitro Edit on Studio β†—οΈŽ View Live Preview f79c28b

Comment on lines +25 to +28
rules = [
{ type = "ESModule", globs = ["**/*.js", "**/*.mjs"]},
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added those here to simplify the documentation and not requiring people to set them only when using the cloudflare_module preset.

The the service worker output these are not going to do anything but they are not breaking anything either (as you can see from this example), that's why I think it's simpler to just include them here, but let me know what you think, I don't mind moving it into the cloudflare_module preset section.

@dario-piotrowicz dario-piotrowicz marked this pull request as ready for review November 11, 2023 23:28
@pi0 pi0 changed the title feat(cloudflare): remove NITRO_EXP_CLOUDFLARE_DYNAMIC_IMPORTS feat(cloudflare-pages): enable code splitting by default Nov 12, 2023
@pi0 pi0 changed the title feat(cloudflare-pages): enable code splitting by default feat(cloudflare-pages, cloudflare-module): enable code splitting by default Nov 12, 2023
@@ -12,7 +12,7 @@ describe("nitro:preset:cloudflare-pages", async () => {
testNitro(ctx, () => {
const mf = new Miniflare({
modules: true,
scriptPath: resolve(ctx.outDir, "_worker.js"),
scriptPath: resolve(ctx.outDir, "_worker.js", "index.js"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we always need a path like this as .../_worker.js/index.js? (double extension seems strange but fine if it is only allowed convention)

Copy link
Contributor Author

@dario-piotrowicz dario-piotrowicz Nov 12, 2023

Choose a reason for hiding this comment

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

yeah it's the only allowed convention unfortunately πŸ˜“

later this might be configurable but for now it has to be like this πŸ˜“

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Lets try on nightly channel!

@pi0 pi0 merged commit 25d07fb into nitrojs:main Nov 12, 2023
4 checks passed
@pi0
Copy link
Member

pi0 commented Nov 12, 2023

This is huge πŸ”₯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants