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

Ensure const modules in async bundles are wrapped #9740

Merged
merged 3 commits into from
May 28, 2024

Conversation

marcins
Copy link
Contributor

@marcins marcins commented May 27, 2024

↪️ Pull Request

This fixes a case where an async module only exports a constant, and it doesn't end up getting registered.

We had a use case where there were lots of async bundles containing just string exports for a PDF renderer, and a big switch case that would dynamically import fonts on demand. These small async bundles weren't registering their exports.

Pseudo example based on the real pattern - where the real values could vary in size between several hundred bytes and 60KB+, hence deferred loading:

// fonts.ts
export const fonts = {
   'abc': () => import('./abc'),
   'def': () => import('./def')
}

// abc.ts
export const value = "abc";


// consumer.ts
import { fonts } from './fonts';

const { value } = await fonts['abc']();

🚨 Test instructions

  • Added new integration test that was failing before the fix.
  • Verified internally that the affected async bundles correctly register their assets

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@marcins marcins requested a review from a team May 27, 2024 23:34
);

// This will fail when the async bundle does not export it's constant
await run(b);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the asset still get inlined in this case? It wouldn't be ideal if you ended up with something like:

import('./async').then(m => console.log('async value'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, it doesn't seem to - I assume the static analysis can't track the usage to that level anyway?

From the bundle in the integration test:

(parcelRequire("8Vicm")).then((m)=>console.log(m.value));

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 additional verification in the integration test.

@marcins marcins added this pull request to the merge queue May 28, 2024
Merged via the queue into v2 with commit 2628e7b May 28, 2024
14 of 17 checks passed
@mischnic mischnic deleted the mszczepanski/inline-const-async-bundle branch May 28, 2024 11:36
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.

3 participants