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

Fix asset path issue for "compound-design-tokens" package #26646

Closed
wants to merge 1 commit into from
Closed

Fix asset path issue for "compound-design-tokens" package #26646

wants to merge 1 commit into from

Conversation

menturion
Copy link

@menturion menturion commented Nov 25, 2023

The fix given here #26069 does not work if one clone the compound-design-tokens package separately and link it within the matrix-react-sdk, because the regex /@vector-im(?:\\|\/)compound-(.*?)(?:\\|\/)/ does not match the paths in case of a cloned & linked package due to the missing substring @vector-im, see below:

Resource paths (on Windows) are e.g. ...

  • Not linked package
    resourcePath: C:\src\matrix-react-sdk\node_modules\@vector-im\compound-design-tokens\icons\chat-solid.svg

  • Cloned & linked package
    resourcePath: C:\src\compound-design-tokens\icons\chat-solid.svg

Problem:
The main problem is not whether absolute vs relative paths but that the compound-design-tokens package does not have a dist or res subfolder to serve the assets so that the existing regex /^.*[/\\](dist|res)[/\\]/ does not match.

Solution:
The solution is to add the missing root folder name for the assets to the above regex, with ...
/^.*[/\](dist|res|compound-design-tokens)[/\]/

I have tested this for both scenarios (linked/not linked) on Windows and it works with the regex statement:

const prefix = /^.*[/\\](dist|res|compound-design-tokens)[/\\]/;

The full part regarding compoundImportsPrefix is in this case obsolete and can be removed in the webpack.config.js.

Signed-off-by: menturion menturion@gmail.com


This change is marked as an internal change (Task), so will not be included in the changelog.

Fixed path issue for "compound-design-tokens" package independent of whether the package is cloned and linked within "matrix-react-sdk" or not.

Resource paths are e.g. ...

- Not linked package -
resourcePath: C:\src\matrix-react-sdk\node_modules\@vector-im\compound-design-tokens\icons\chat-solid.svg

- Cloned & linked package -
resourcePath: C:\src\compound-design-tokens\icons\chat-solid.svg

Problem: 
The "compound-design-tokens" package does not have a 'dist' or "res" subfolder for the assets so that the regex /^.*[/\\](dist|res)[/\\]/ does not match. 

Solution: 
Add the missing root folder name for the compound tokens assets to the regex in the "webpack.config.js" to match the paths, with resulting regex... 

/^.*[/\\](dist|res|compound-design-tokens)[/\\]/

Tested both scenarios (linked/not linked) on Windows.
@menturion menturion requested a review from a team as a code owner November 25, 2023 09:58
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Nov 25, 2023
@richvdh richvdh added the T-Task Tasks for the team like planning label Nov 27, 2023
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

This looks sensible to me.

However, I'm afraid we don't accept contributions signed off under a pseudonym, per CONTRIBUTING.md, so we'll need your real name to be able to accept this.

If you prefer, you can provide this privately by emailing dco@matrix.org, though our contributing guide doesn't seem to mention this (#26654).

@menturion
Copy link
Author

Hi Richard, many thanks for the info and your support.
I have filed the issue here #26657 which should be fixed IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants