search

FelixSchwarz / mjml-python

Python implementation for MJML - a framework that makes responsive-email easy
MIT License
72 stars 16 forks source link

CSS improvements #45

Closed caseyjhol closed 8 months ago

caseyjhol commented 9 months ago
caseyjhol commented 9 months ago

@FelixSchwarz I'm trying to upgrade to css-inline v0.11.0 to take advantage of performance improvements added in v0.10.x, but it requires using the keep_link_tags and keep_style_tags arguments, which older versions of css-inline don't support. 0.8.3 is the latest version that supports Python 3.6, so we're getting "unexpected keyword argument" errors when trying to use it in the 3.6 tests. Not sure how you'd like to handle this. I think a try/except block might be our only option (besides dropping support for v3.6, which seems extreme), but maybe you have another idea.

Stranger6667 commented 9 months ago

Btw, I’d be curious how much the update improves performance in your scenario - I have very few benchmarks so it would be cool to learn about real life effect of all that performance changes :)

caseyjhol commented 9 months ago

@Stranger6667 Just to consider all options, how feasible would it be to add back support for Python v3.6 to css-inline?

Stranger6667 commented 9 months ago

@caseyjhol Supporting Python 3.6 requires downgrading pyo3 to 0.15.2 which does not support Python 3.11 and the number of bugfixes between the current pyo3 version and that one is quite big. It does not feel reasonable for css-inline to downgrade that much, but I guess, generally, pyo3==0.15.2 should be compatible with the current binding's implementation. I.e. it could be a fork / different branch that could use older pyo3 and released under a different name on PyPI (e.g. css-inline-py36) and used as a conditional dependency for Python 3.6 only.

FelixSchwarz commented 9 months ago

0.8.3 is the latest version that supports Python 3.6, so we're getting "unexpected keyword argument" errors when trying to use it in the 3.6 tests. Not sure how you'd like to handle this.

I think we can make a point that css inlining is only support for Python >= 3.7. We are still using mjml in production with Python 3.6 - probably until RHEL 7 support ends and I can justify spending on mjml-python more easily if we can use mjml in our infrastructure.

However we don't need css inlining for the Python 3.6 deployment, so this PR seems like a good reason to raise that requirement. I would release the next version as 0.10.0 as this is kind of a breaking change for Python 3.6 users but everyone needs to upgrade at some point.

caseyjhol commented 9 months ago

That makes sense to me (to lock CSS support to Python 3.7 starting in v0.10.0). If we get someone requesting Python 3.6 CSS inlining support, we can reconsider a different approach.

If we want to be really granular about it, we could release my CSS fixes in v0.9.2 (so Python 3.6 users could still use that version for CSS support). And then we could follow-up with a css-inline upgrade in v0.10.0. I think that probably makes sense. I'll split that out into a separate PR (https://github.com/FelixSchwarz/mjml-python/pull/46).

caseyjhol commented 8 months ago

@FelixSchwarz Any thoughts here? These are the last items preventing from moving forward with a prototype for some users. Thanks!

FelixSchwarz commented 8 months ago

Your branch contains some "wip-like" commits. I tried to extract the meaningful changes but I'm always getting some test failures locally. May I ask you to rebase/squash your commits on top of the latest main? I don't have any objections to code, so this just about getting your changes to apply (and a cleaned-up commit history).

caseyjhol commented 8 months ago

Sure - can clean those up now. I'm used to doing a bunch of commits and then just doing a squash/merge.

FelixSchwarz commented 8 months ago

Thank you. Are there more features you would like to add or should we just release version 0.10?

caseyjhol commented 8 months ago

I think that's it for now - I'm good with a 0.10.0 release. Thanks!