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

refactor(atomic): replace @stencil/store with in-house implementation #4814

Merged
merged 26 commits into from
Jan 16, 2025

Conversation

alexprudhomme
Copy link
Contributor

@alexprudhomme alexprudhomme commented Jan 2, 2025

https://coveord.atlassian.net/browse/KIT-3815

Why replace this dependency ?

We are replacing stencil with lit and @stencil/store has some functionalities depending on stencil internal functions. It won't work once we begin switching.

The implementation is here #4814 (comment)

Why all the changes for isAppLoaded ?

@stencil/store has a hidden functionality we were unknowingly depending on. There is an stencil subscription that causes automatic rerenders. The subscription listens to every component 'getting' a certain part of the state. It registers those components and whenever that same part of the state is 'set', it triggers a new render of those components. We were depending on it for the loadingFlags part of the state.

In this PR I added createAppLoadedListener which takes in a callback and calls it whenever loadingFlags are empty. In every component that needs to listen to the loading flags, we add a @State isAppLoaded and create the listener to that internal state. Whenever that internal state is re triggered, it rerenders the component.

Alternatives to createAppLoadedListener

  1. We could create a decorator. That would mean implement the decorator in both Stencil & Lit. We can always revisit this whenever we implement Lit and figure out something better.
  2. We could use Lit signals for this. It is exactly the signal use case.

@alexprudhomme alexprudhomme requested a review from a team January 2, 2025 16:37
@alexprudhomme alexprudhomme requested review from a team as code owners January 2, 2025 16:37
@alexprudhomme alexprudhomme requested review from mikegron, dguerinCoveo, olamothe, fpbrault, y-lakhdar and louis-bompart and removed request for a team January 2, 2025 16:37
Copy link

github-actions bot commented Jan 2, 2025

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

Live demo links

Bundle Size

File Old (kb) New (kb) Change (%)
case-assist 243.8 243.8 0
commerce 355 355 0
search 414.9 414.9 0
insight 406.2 406.2 0
recommendation 256.1 256.1 0
ssr 408.8 408.8 0
ssr-commerce 372.7 372.7 0

SSR Progress

Use case SSR (#) CSR (#) Progress (%)
search 39 44 89
recommendation 0 4 0
case-assist 0 6 0
insight 0 27 0
commerce 0 15 0
Detailed logs search : buildInteractiveResult
search : buildInteractiveInstantResult
search : buildInteractiveRecentResult
search : buildInteractiveCitation
search : buildGeneratedAnswer
recommendation : missing SSR support
case-assist : missing SSR support
insight : missing SSR support
commerce : missing SSR support

Base automatically changed from store-refactor to master January 15, 2025 20:18
@alexprudhomme alexprudhomme added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025
@alexprudhomme alexprudhomme added this pull request to the merge queue Jan 16, 2025
@alexprudhomme alexprudhomme removed this pull request from the merge queue due to a manual request Jan 16, 2025
GitHub Actions Bot and others added 2 commits January 16, 2025 13:04
@alexprudhomme alexprudhomme added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025
@alexprudhomme alexprudhomme added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 16, 2025
@alexprudhomme alexprudhomme added this pull request to the merge queue Jan 16, 2025
Merged via the queue into master with commit cc9cd0f Jan 16, 2025
124 checks passed
@alexprudhomme alexprudhomme deleted the replace-stencil-store branch January 16, 2025 18:38
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.

4 participants