Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore(atomic): update getAssetPath #4946

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

chore(atomic): update getAssetPath #4946

wants to merge 13 commits into from

Conversation

y-lakhdar
Copy link
Contributor

@y-lakhdar y-lakhdar commented Feb 6, 2025

Update the logic to handle asset paths without the need of Stencil. The logic is based on Stencil/core's getAssetPath function, where the resourceUrl is dynamically determined based on how the app is built.

Logic

When served from a CDN, the resourceUrl is computed based on the file location (import.meta.url). This value is injected at build time via a TypeScript custom transformer.
Otherwise, it is computed from the document location.

Test Configuration Update:

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

Copy link

github-actions bot commented Feb 6, 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.7 243.7 0
commerce 355 355 0
search 415 415 0
insight 406.2 406.2 0
recommendation 255.9 255.9 0
ssr 408.8 408.8 0
ssr-commerce 372.7 372.7 0

@alexprudhomme
Copy link
Contributor

is this really a feature ?

@y-lakhdar y-lakhdar marked this pull request as ready for review February 12, 2025 20:22
@y-lakhdar y-lakhdar requested a review from a team as a code owner February 12, 2025 20:22
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

How does the assetPath works for NPM user today, and how would it works afterward?

Co-authored-by: Louis Bompart <lbompart@coveo.com>
@y-lakhdar
Copy link
Contributor Author

How does the assetPath works for NPM user today, and how would it works afterward?

For NPM users today, if the iconAssetsPath attribute on the search interface is left unchanged, the getAssetPath method retrieves assets from the default folder using a relative path (e.g., /assets/[icon] for icons).

This behavior remains the same moving forward. Since the RESOURCE_URL environment variable is only set when building for CDN, the asset base URL continues to be determined based on the browser’s location.

@alexprudhomme
Copy link
Contributor

alexprudhomme commented Feb 17, 2025

How does the assetPath works for NPM user today, and how would it works afterward?

For NPM users today, if the iconAssetsPath attribute on the search interface is left unchanged, the getAssetPath method retrieves assets from the default folder using a relative path (e.g., /assets/[icon] for icons).

This behavior remains the same moving forward. Since the RESOURCE_URL environment variable is only set when building for CDN, the asset base URL continues to be determined based on the browser’s location.

So technically RESOURCE_URL could be any string or any variable since it is undefined in the npm build and replaced in the cdn one ?

Also, this is not a feature, you should change the pr title.

@y-lakhdar y-lakhdar changed the title feat(atomic): update getAssetPath fix(atomic): update getAssetPath Feb 17, 2025
@y-lakhdar y-lakhdar changed the title fix(atomic): update getAssetPath chore(atomic): update getAssetPath Feb 17, 2025
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

I think that's fine.
However, QA Approval will be mandatory for this one

@y-lakhdar y-lakhdar enabled auto-merge February 18, 2025 19:39
@y-lakhdar y-lakhdar disabled auto-merge February 18, 2025 19:40
Copy link
Contributor

@alexprudhomme alexprudhomme left a comment

Choose a reason for hiding this comment

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

lets try it !

@y-lakhdar y-lakhdar requested review from lvu285 and wmannard February 18, 2025 19:43
Copy link
Contributor

@fpbrault fpbrault left a comment

Choose a reason for hiding this comment

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

🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants