Skip to content

fix(dev): Append styles to head even for duplicates, fix key construction #683

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

Merged

Conversation

rjz-avaleo
Copy link
Contributor

@rjz-avaleo rjz-avaleo commented Mar 7, 2025

Description

This PR fixes two errors. One, simpler was introduced in this commit: 766b521 and it changed
const key = 'css__${options.name}__' + exposeItemName;
to
const key = 'css__' + options.name + '__' + exposeItemName;
which causes an error, because options.name should be resolved and not passed as a string.

The second error that this PR fixes is more complicated and I didn't think about such a case, when I introduced this feature in your project for the first time here: #548

Multiple remote components can have the same path to a CSS file with styles, becasue they can come from the same remote application (that's actually a case we have in our project). The styles between components differ very little (if at all), but the majority of styles are shared between those components and they are "base" styles like for buttons, inputs, etc. The build of such a case will consist of different federated modules, but a single CSS file.

In such a case there's a problem caused by this code fragment:
if (href in seen) return;

For styles that are appended to head, it's a correct condition, because we don't want the styles to appear more than once. However if the styles need to be appended to a Shadow Root for multiple different components, each having its own Shadow Root, then this causes a problem, because only THE FIRST loaded component will add the styles href to the window object. All the other components that want to add THE SAME styles href won't do that because of if (href in seen) return;. And I think it's a perfectly fine scenario with Shadow DOM usage.

That's why I moved the part of the code acting when dontAppendStylesToHead flag is set to true to BEFORE the if (href in seen) return; part.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Code of Conduct and follow the Commit Convention guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Jiannan-dev
Copy link
Collaborator

Sry,it looks like there's an issue that's been fixed that's causing a conflict,resolve the conflict please
@rjz-avaleo

@rjz-avaleo rjz-avaleo force-pushed the fix-styles-not-appended-to-head branch from 6a436d9 to d4200bc Compare March 14, 2025 09:25
@rjz-avaleo
Copy link
Contributor Author

rjz-avaleo commented Mar 14, 2025

@ruleeeer Ok, so it seems that someone already fixed one of the things I did in this PR. So there's only this second fix - "the more complicated" one. Should I edit the PR description to remove mentions of this first "easy" fix since it's not relevant anymore? Maybe also remove the mention from the commit message?

@Jiannan-dev
Copy link
Collaborator

Thanks, I can't wait to release a new version!

@Jiannan-dev Jiannan-dev merged commit 8381233 into originjs:main Mar 15, 2025
8 checks passed
@rjz-avaleo
Copy link
Contributor Author

Great! Thank you!

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.

2 participants