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

Digest dynamic/async imports in javascript assets #6003

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomtaylor
Copy link
Contributor

@tomtaylor tomtaylor commented Dec 7, 2024

esbuild supports ES6 dynamic module imports, but they're not currently compatible with the mix phx.digest process as the import paths aren't digested.

This PR adds support for digesting any async and promise based imports inside a JS file, so a developer can use these without giving up cache busting, etc. Dynamic imports are great for front end performance as expensive libraries can be loaded on-demand rather than up front.

Before this PR, the following code would be the same after mix phx.digest runs. A digested file will load a non-digested other-module.js, and so not benefit from cache busting, etc.

async function onClick() {
  const { usefulFunction } = await import("./other-module.js");
  usefulFunction();
}

After this PR mix phx.digest will switch the import path for the digested path, e.g.

async function onClick() {
  const { usefulFunction } = await import("./other-module-2ddf92b4c94e25bd296b2df90848a564.js");
  usefulFunction();
}

Note: esbuild in Phoenix doesn't use async imports out of the box, it need to be enabled by changing the args to include following flags: --chunk-names=js/chunks/[name]-[hash] --splitting --format=esm. (Technically, it does, but the promise just resolves instantly to content that's in the same file.) I think there's an argument for changing the default flags to do this, but I understand if that might add more complexity than desired to the default asset pipeline.

I think there is value in supporting dynamic imports in the digester even if we don't flip the defaults, because it allows users who want to implement their own esbuild config to benefit from splitting + dynamic imports, without having to throw out the Phoenix digesting process entirely. Throwing out the Phoenix digest process is fairly annoying, as it means finding a solution for digesting the non-JS assets, which esbuild doesn't really support.

This PR doesn't digest any static chunk imports, eg. import { foo } from "./chunks/chunk-abcdef.js". These already have a hash appended and so the benefit would mostly aesthetic, but it's probably still worth implementing to minimise any surprises.

Anyway, let me know what you think and whether this is something you'd like to add to Phoenix. If there's support for it, I suggest I add some documentation, support for chunk imports and handle the optional options argument that import() can support.

@tomtaylor tomtaylor changed the title Digest async imports in javascript assets Digest dynamic/async imports in javascript assets Dec 7, 2024
@SteffenDE
Copy link
Contributor

I think I remember esbuild also automatically appending hashes to dynamic imports in one project I worked on using a config like you've shown. I cannot check at the moment though.

This approach feels a bit brittle with the regex. Currently, it wouldn't detect the import when it spans multiple lines, I think:

function promiseImport() {
  import("./app.js")
    .then((app) => {
       console.log(app);
    });
}

So I'm not sure it wouldn't be better to just leave that job to esbuild and maybe enable the splitting config by default for new projects?

@tomtaylor
Copy link
Contributor Author

I think I remember esbuild also automatically appending hashes to dynamic imports in one project I worked on using a config like you've shown.

esbuild will add a checksum to e.g. chunk-ABCDEFGH.js files, where it's deciding the name of the chunk based on how it decides to collate the imports, however it won't do that for dynamic imports where a named file is being imported.

This approach feels a bit brittle with the regex. Currently, it wouldn't detect the import when it spans multiple lines, I think:

Agree it's not ideal (the 'proper' way would be to use an AST transformation). But then neither is what we're already doing with source maps. I think the Regex could catch all the common scenarios, including line breaks, and there's a natural place to extend the test cases we already have.

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