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

LinkTagCssSource now also downloads content from @import declarations. #419

Merged
merged 20 commits into from
Jan 29, 2025

Conversation

whorchner
Copy link
Contributor

PreMailer:

  • fixed obsolete DescendentsAndSelf to DescendantsAndSelf

PreMailer:
- fixed obsolete DescendentsAndSelf to DescendantsAndSelf
Copy link
Contributor

@martinnormark martinnormark left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I've added a few requests for improvement.

PreMailer.Net/PreMailer.Net/Sources/LinkTagCssSource.cs Outdated Show resolved Hide resolved
PreMailer.Net/PreMailer.Net/PreMailer.Net.csproj Outdated Show resolved Hide resolved
PreMailer.Net/PreMailer.Net/PreMailer.Net.csproj Outdated Show resolved Hide resolved
PreMailer.Net/PreMailer.Net/Sources/LinkTagCssSource.cs Outdated Show resolved Hide resolved
W Hörchner added 2 commits January 21, 2025 11:10
Extracted the import logic to it's own ImportRuleCssSource class.

PreMailer:
- fixed obsolete DescendentsAndSelf to DescendantsAndSelf
@martinnormark
Copy link
Contributor

@whorchner I messed up my sync to your branch. I have the changes locally, will re-create. Sorry!

@martinnormark
Copy link
Contributor

martinnormark commented Jan 22, 2025

@whorchner It is easiest if you can give me push access to your fork. The changes is also in the pr419 branch and I accidentally pushed to main. Doh, really bad late-night coding here :neckbeard:

You can provide access with the checkbox in the sidebar as explained here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

@whorchner
Copy link
Contributor Author

I checked the box.

@martinnormark
Copy link
Contributor

martinnormark commented Jan 23, 2025

For whatever reason, I do not have permission to push.

From your fork, can you try to do this:

git fetch upstream pr419
git merge upstream/pr419

@whorchner
Copy link
Contributor Author

fatal: 'upstream' does not appear to be a git repository
When I try the fetch command with 'origin' (what it strangely says in the email from github) I also get an fatal: couldn't find remote ref pr419.

@whorchner
Copy link
Contributor Author

On my github I see the changes are gone. In visual studio local I have 11 outgoing changes. I can push or sync. Accoording to VS there are no incomming changes.

@whorchner
Copy link
Contributor Author

Might it be a problem that this PR is closed?

@martinnormark
Copy link
Contributor

PR being closed is not a problem. You should be able to push your changes.

@martinnormark
Copy link
Contributor

fatal: 'upstream' does not appear to be a git repository

Can you try to share the output of git remote -v?

@whorchner
Copy link
Contributor Author

The output of 'git remote -v':
origin https://github.com/whorchner/PreMailer.Net (fetch)
origin https://github.com/whorchner/PreMailer.Net (push)

@whorchner
Copy link
Contributor Author

I pushed the changes and I see them again on the github fork

@martinnormark martinnormark reopened this Jan 23, 2025
@martinnormark
Copy link
Contributor

I pushed the changes and I see them again on the github fork

Great, now I could also push to your fork. So the PR is correct now, I refactored the import css source and added more tests. Could you try to run it with the emails you have with these nested imports?

@whorchner
Copy link
Contributor Author

We have multiple stylesheet which are used separately but also combined. Some stylesheets import a variables.css stylesheet. When getting all imports the variables.css would be downloaded multiple times. So, I had to add the _importList.ContainsKey check.

…eet in their import list, checking if the url already exists in the _importList is necessary.
@whorchner
Copy link
Contributor Author

Since you're the maintainer of PreMailer it is your call and I'm fine with it. My implementation breaking the ICssSource contract seems a matter of opinion, but now you have a 'source' class without the ICssSource interface, implementing a GetCss method not adhering any contract.

@martinnormark
Copy link
Contributor

martinnormark commented Jan 23, 2025

We have multiple stylesheet which are used separately but also combined. Some stylesheets import a variables.css stylesheet. When getting all imports the variables.css would be downloaded multiple times. So, I had to add the _importList.ContainsKey check.

So does it work? I will try to make time to add more tests, it should test the logic more sufficiently especially not downloading sources twice, the order it appends the CSS blocks.

Since you're the maintainer of PreMailer it is your call and I'm fine with it. My implementation breaking the ICssSource contract seems a matter of opinion, but now you have a 'source' class without the ICssSource interface, implementing a GetCss method not adhering any contract.

I played around a lot, e.g. making ICssSource have a generic type to specify an options class that goes into GetCss. In the end, mostly because of how GetCss is called on a list of sources, I decided that the import source is different since it will never be available as a source at the root document so it should not try to be an ICssSource.

The GetCss method could be renamed to not resemble traces of ICssSource.

W Hörchner and others added 5 commits January 23, 2025 11:34
- Implemented tests for loading CSS imports recursively up to two levels.
- Added caching mechanism to prevent multiple downloads of the same stylesheet.
- Ensured import order is preserved when processing multiple imports.
- Handled circular imports to avoid infinite loops during CSS retrieval.
- Enhanced test coverage for ImportRuleCssSource functionality.
@whorchner
Copy link
Contributor Author

Hi Martin,

I think I accidentally commit changes to this PR. I hoped to get a chance to create new PR, but that was not the case. I'm not sure how to do that, so the committed changes have to wait. If you know how to remove the commits from this PR, you are welcome to remove them.

@martinnormark
Copy link
Contributor

Try to run git reset a5c6cf67eddf26d2d5b30940553906610202b3ba that should reset the branch back to a5c6cf6.

@martinnormark
Copy link
Contributor

Do take a copy of the changes and keep outside of the repo.

@whorchner
Copy link
Contributor Author

Thanks for the tip. It does reset locally, but it does not remove the commit from the PR.

@martinnormark
Copy link
Contributor

Remember to git push and if it fails, then do git push --force.

@whorchner
Copy link
Contributor Author

Thanks. That worked (--force). The PR is now back to wat it should be.

@martinnormark martinnormark merged commit adb1b22 into milkshakesoftware:main Jan 29, 2025
4 checks passed
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