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

Dedup stylesheet href #90

Merged
merged 3 commits into from
Nov 1, 2020

Conversation

sbeckeriv
Copy link
Contributor

Dearest Maintainer,

First thank you for your work on this project. I am comparing this to
the Roadie gem for ruby. It inlines css into html emails. I have a
really fun email that keeps including a script tag. The same dup script
tag. I have updated your code to dedup the hrefs you are loading.

Thanks again. Your command line tool takes 3.1 seconds where the
Roadie gem takes about 2 minutes.

Dictated but not reviewed,
Becker

Dearest Maintainer,

First thank you for your work on this project. I am comparing this to
the Roadie gem for ruby. It inlines css into html emails. I have a
really fun email that keeps including a script tag. The same dup script
tag. I have updated your code to dedup the hrefs you are loading.

Thanks again. Your command line tool takes 3.1 seconds where the
Roadie gem takes about 2 minutes.

Dictated but not reviewed,
Becker
I noticed the clippy error in the pr.
src/lib.rs Outdated
.borrow()
.get("href")
.as_ref()
.unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this need to handle it not being there.

Copy link
Owner

@Stranger6667 Stranger6667 left a comment

Choose a reason for hiding this comment

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

Hi @sbeckeriv

Cool! Thank you for your contribution :) I left some performance-related comments (minor ones, though)

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@Stranger6667
Copy link
Owner

Thanks again. Your command line tool takes 3.1 seconds where the
Roadie gem takes about 2 minutes.

Wow! That is great! :)

@Stranger6667
Copy link
Owner

I will merge it as is and then apply some performance tweaks :)

@Stranger6667 Stranger6667 merged commit 49b26bb into Stranger6667:master Nov 1, 2020
@sbeckeriv
Copy link
Contributor Author

Suggestions looks good. It has been a busy few days. Thanks for merging and applying. Should the node built be passing? I could not figure that out.

@Stranger6667
Copy link
Owner

Should the node built be passing? I could not figure that out.

Yep, this one is quite strange, probably it might be caused by some WASM-related dependency updates, but the TypeScript tests pass, which is good enough to make a new release :) I'll look at the issue later

@sbeckeriv sbeckeriv deleted the dedup-style-sheets branch October 20, 2021 18:27
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.

None yet

2 participants