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

Use ES modules and dynamic import for Webpack latest builds #16620

Merged
merged 2 commits into from
May 30, 2023

Conversation

steverep
Copy link
Member

Proposed change

Output ESM for chunks instead of classic scripts, and have Webpack load them with import() instead of script elements. Technically, this is still "experimental" in Webpack, but it's been implemented for quite some time, seems pretty stable in the release notes, and this change is overdue I think. A few details:

I did try to compare page load time for the demo content and this change was pretty much always faster, maybe by about 1.5-2x (with large variability obviously).

As a sign of stability, the bundle is more or less identical aside from the format change and considerably less injected runtime code:

Entrypoint Old New Change
app 248K 246K -1%
core 19K 18K -6%
custom-panel 34K 33K -3%
authorize 271K 270K -0%
onboarding 294K 293K -0%
Total Bundle 18M 18M -0%

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@steverep steverep added the Build Related to building the code label May 25, 2023
@steverep
Copy link
Member Author

Way too late here to fix the mocha issue... 😢

bramkragten
bramkragten previously approved these changes May 25, 2023
@steverep
Copy link
Member Author

Test failure is another instant-mocha shortcoming. I'll give a day for them to publish my fix before overriding it here.

@steverep
Copy link
Member Author

Crickets at instant-mocha so far... I just overrode it.

@steverep steverep requested a review from bramkragten May 30, 2023 18:20
@bramkragten bramkragten enabled auto-merge (squash) May 30, 2023 18:26
@bramkragten bramkragten merged commit 7b350e3 into dev May 30, 2023
@bramkragten bramkragten deleted the output-es-modules branch May 30, 2023 18:27
@bramkragten
Copy link
Member

It seems like our background workers no longer work (like the datatable filtering and sorting https://github.com/home-assistant/frontend/blob/dev/src/components/data-table/sort-filter.ts) after this PR.

Will check tomorrow if I can find the issue, otherwise I will have to revert this for the beta.

@steverep
Copy link
Member Author

Hmmm... are you seeing errors? They should be loaded using import() just like the rest unless the default output.workerChunkLoading is screwed up. I'll check it out but might not be able to get to it until tomorrow.

@bramkragten
Copy link
Member

No errors... :-(
It seems like the promise is just not resolved of the worker. The problem might be related to comlink...

@bramkragten
Copy link
Member

bramkragten commented May 30, 2023

const worker = new Worker(new URL("./sort_filter_worker", import.meta.url))
=> Worker {onmessage: null, onerror: null}
const wrapped = Comlink.wrap(worker);
=> Proxy(Function) {length: 0, name: 'target', prototype: {…}}
await wrapped.sortData([])

Doesn't resolve

@bramkragten
Copy link
Member

I'm just gonna revert this for now, if we fix it we can add it all back in :-)

steverep pushed a commit that referenced this pull request May 30, 2023
#16679)

Revert "Use ES modules and dynamic import for Webpack latest builds (#16620)"

This reverts commit 7b350e3.
@bramkragten
Copy link
Member

@steverep
Copy link
Member Author

Yeah that's defaulting it to import as expected, so that's not it. Webpack has a whole separate web worker target as if they expect them always to be separate builds, but it's not clear that actually does anything special to the output. I'll have to look at the actual chunk code I guess.

The service worker was okay right? That part I did test and it seemed to be caching fine.

@steverep steverep restored the output-es-modules branch May 30, 2023 22:10
@steverep steverep deleted the output-es-modules branch May 31, 2023 02:46
@steverep
Copy link
Member Author

So similar to entry chunks, it doesn't look like the worker chunks are fully converted to ESM yet, which means its exports probably don't work. I'll see if they can just be loaded the old way without breaking the rest.

@steverep
Copy link
Member Author

Scratch that... comlink seems to be the issue because the QR worker looks to be okay. Can you confirm?

@steverep
Copy link
Member Author

steverep commented Jun 3, 2023

Okay after researching this a bit, it seems we actually have 2 problems regarding ESM workers:

  1. comlink causes the hangs. There are 2 possible matching issues already filed.
  2. Firefox won't support them until 114 is released (111 has module support behind a flag, and 113 added dynamic import support)

So we either find the comlink issue and patch it, and then restrict Firefox, or fallback to "classic" workers for now. I discussed how best to do the latter with Webpack developer (webpack/webpack#17298). Best approach is to build the workers separately because Webpack really can't handle multiple chunk formats in one build.

Thoughts?

@balloob
Copy link
Member

balloob commented Jun 3, 2023

Given that Firefox won’t support it for a while, I suggest we keep workers with 0 imports. That way comlink will also continue to work right?

@steverep
Copy link
Member Author

steverep commented Jun 4, 2023

If we implement something like #16506, then the Firefox restriction isn't too long a wait. Presuming it works well in 114 (release this Tuesday), ESR will then switch to 115 and drop 102 completely after a few months (Firefox release schedule).

I'm still trying to prove comlink is the issue and not Webpack. I see some code evidence that it might actually be Webpack. Need to dive a little deeper. But yeah the solutions regardless are either no imports or "classic" using importScripts as it is now.

@balloob
Copy link
Member

balloob commented Jun 4, 2023

I do not want to kick out all current Firefox versions from latest. So we should wait then until ESR has been released.

@steverep
Copy link
Member Author

steverep commented Jun 4, 2023

No that won't happen. Working around the webpack/comlink issue will moot the Firefox issue.

@steverep
Copy link
Member Author

steverep commented Jun 9, 2023

  1. I've got working workers now (Firefox too). PR coming soon.
  2. Webpack is at fault, not comlink. Workers never get past importing any external chunks. Will file an issue with them.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Build Related to building the code cla-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants