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

bug: Liquid as external dependency stopped working with version 4.6.0 and above #700

Closed
3 tasks done
vvision opened this issue May 11, 2023 · 4 comments · Fixed by #712
Closed
3 tasks done

bug: Liquid as external dependency stopped working with version 4.6.0 and above #700

vvision opened this issue May 11, 2023 · 4 comments · Fixed by #712
Labels
accepted Issues with this label are issues that the core team has accepted on to the roadmap. feature Issues that describe new features. released

Comments

@vvision
Copy link

vvision commented May 11, 2023

Prerequisites

Liquid version

4.6.0 and above

Framework bindings

  • React + Vite
  • Preact + preact-custom-element + Vite

Current behavior

Upgrading from Liquid 4.5.1, we are no longer able to exclude Liquid from the bundle of our webcomponents.
Our existing configuration does not work anymore starting with Liquid 4.6.0 and above.
Bundles are built with Vite.

Expected behavior

In our project, we expect Liquid components to be handled and defined by the main frontend.
We have a set of webcomponents that are integrated dynamically in our platform.

We expect these components to no bundle Liquid components, to keep them small.

We had a working configuration, it seems either we need to improve it to make it work with the last version of Liquid, or this is no longer doable the way we did it. So not sure if its a liquid "bug", a missing feature, or a missing configuration or wrong implementation on our side.

Do you have any recommandation regarding this subject, ie: declaring and using Liquid as an external dependency.

Steps to reproduce

In the repository linked, you will find a simplified project which should give you an idea of what we are doing and trying to achieve.

Code reproduction URL

https://github.com/vvision/bug-liquid-external

Additional information

No response

@vvision vvision added bug An issue describing unexpected or malicious behaviour. triage An issue that needs assessment to determine its validity and urgency labels May 11, 2023
@borisdiakur
Copy link
Contributor

borisdiakur commented May 16, 2023

@vvision Thanks for submitting this issue and putting so much effort in it. 🙏
I'm looking into it. It's a tricky one. 😬

borisdiakur added a commit that referenced this issue May 16, 2023
…ne custom elements

Since liquid v4.6.0 the includeDefineCustomElements option set to true. With the includeDefineCustomElements option set to true, you don't need to define the custom elements from liquid react yourself. The custom elements get registered with the Custom Elements Registry as soon as imported. We didn't see this as a breaking change, because calling the defineCustomElements helper method manually would not break anything, even if the elements were already defined. However, things get tricky, when bundling liquid components into another library:
With the includeDefineCustomElements option set to true, the react.js and vue.js files within liquid import the defineCustomElements helper and execute it. The defineCustomElements helper imports the bootstrapLazy helper which in turn includes dynamic import statements for all components. Now, a bundler cannot know which component will be lazy loaded eventually, so it includes all components as chunks into the bundle. The app that consumes the library can load only what's needed, but the bundle is still unnecessary big, including all Liquid components, even if not used.
In order to solve this issue, we add additional entry points to the output targets, which do not include the defineCustomElements helper utility. This allows to manually register custom elements on a as-needed basis, and fixes issues with the bundling, because dynamic imports are not used in the code.

Resolves #700
@borisdiakur borisdiakur added accepted Issues with this label are issues that the core team has accepted on to the roadmap. feature Issues that describe new features. in progress This label indicates that the issue is currently being worked on. and removed bug An issue describing unexpected or malicious behaviour. triage An issue that needs assessment to determine its validity and urgency labels May 16, 2023
github-actions bot pushed a commit that referenced this issue May 17, 2023
# [5.5.0](v5.4.1...v5.5.0) (2023-05-17)

### Features

* **output-target:** add output target entries which do not auto define custom elements ([6077647](6077647)), closes [#700](#700)
@borisdiakur
Copy link
Contributor

borisdiakur commented May 17, 2023

Hey @vvision 👋

Good news. It's not a bug. It's a feature.

It's not a bug, it's a feature!

First of all, Liquid should not be external: You want to include the component code in your bundle. => You should remove the external config option.

Now, with Liquid 4.6.0, lots of chunks, which are actually never loaded in the browser, end up in your dist folder. So, why is the tree-shaking not working? Well, it kind of is working, because Rollup figures that all chunks are needed eventually, and the chunks are loaded in the browser only when needed. So why does Rollup "assume" that all chunks are needed? This is related to the includeDefineCustomElements option of the react output target!

Since liquid v4.6.0 the includeDefineCustomElements option set to true. With the includeDefineCustomElements option set to true, you don't need to define the custom elements from liquid react yourself. The custom elements get registered with the Custom Elements Registry as soon as imported. We didn't see this as a breaking change, because calling the defineCustomElements helper method manually would not break anything, even if the elements were already defined.

However things get tricky, when bundling liquid components into another library:
With the includeDefineCustomElements option set to true, the react.js and vue.js files within liquid import the defineCustomElements helper and execute it. The defineCustomElements helper imports the bootstrapLazy helper which in turn includes dynamic import statements for all components. Now, a bundler cannot know, which component will be lazy loaded eventually, so it includes all components as chunks into the bundle. The app that consumes the library can load only what's needed, but the bundle is still unnecessary big, including all Liquid components, even if not used.

In order to solve this issue, we have added additional entry points to the output targets, which do not include the defineCustomElements helper utility. This allows you to manually register custom elements on a as-needed basis, and fixes issues with the bundling, because dynamic imports are not used in the code. How to use this new feature is documented under Manually defining custom elements.

TLDR:
import { LdButton, LdTypo } from '@emdgroup-liquid/liquid/dist/react-define-excluded'

Please note that the feature is available in the newest version of Liquid. There is a major version bump which you need to take care of, but it should be really straight forward, as the breaking change in v5 is only relevant for a very special use case of liquid.

Let me know if something is still not working for you. 🤓

@borisdiakur
Copy link
Contributor

🎉 This issue has been resolved in version 5.5.0 🎉

The release is available on:

📦🚀

@borisdiakur borisdiakur added released and removed in progress This label indicates that the issue is currently being worked on. labels May 17, 2023
@vvision
Copy link
Author

vvision commented May 25, 2023

Hey @borisdiakur 👋,

Thanks for all the explanations and the great work.
I've implemented the proposed solution and it works flawlessly !

Many thanks from our team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issues with this label are issues that the core team has accepted on to the roadmap. feature Issues that describe new features. released
Projects
None yet
2 participants