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

Does not work with className of fade:not(.show) #8

Closed
titanism opened this issue Feb 9, 2023 · 11 comments
Closed

Does not work with className of fade:not(.show) #8

titanism opened this issue Feb 9, 2023 · 11 comments

Comments

@titanism
Copy link

titanism commented Feb 9, 2023

If you have a className value of fade:not(.show) it will result in this error:

SyntaxError: Invalid regular expression: /(?:^|\s)fade:not(\s(?:.*?\s)?show)(?:\s|$)|(?:^|\s)show)\s(?:.*?\s)?fade:not((?:\s|$)/: Unmatched ')'
    at new RegExp (<anonymous>)
    at expandMatcher (/Users/user/Projects/email-templates/node_modules/.pnpm/posthtml-match-helper@1.0.3_posthtml@0.16.6/node_modules/posthtml-match-helper/index.js:37:28)
    at module.exports (/Users/user/Projects/email-templates/node_modules/.pnpm/posthtml-match-helper@1.0.3_posthtml@0.16.6/node_modules/posthtml-match-helper/index.js:144:11)
    at /Users/user/Projects/email-templates/node_modules/.pnpm/posthtml-inline-css@1.2.3_posthtml@0.16.6/node_modules/posthtml-inline-css/lib/inlineCss.js:30:62
    at Array.forEach (<anonymous>)
    at /Users/user/Projects/email-templates/node_modules/.pnpm/posthtml-inline-css@1.2.3_posthtml@0.16.6/node_modules/posthtml-inline-css/lib/inlineCss.js:29:22
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
@titanism
Copy link
Author

titanism commented Feb 9, 2023

You can fix this via escape-string-regexp package:

        attributes.class = new RegExp(getCombinations(className.split(".")).map(function(order){
					return "(?:^|\\s)" + order.map(o => escapeStringRegexp(o)).join("\\s(?:.*?\\s)?") + "(?:\\s|$)";
				}).join("|"));

I am submitting a PR now.

titanism added a commit to titanism/posthtml-match-helper that referenced this issue Feb 9, 2023
@titanism
Copy link
Author

titanism commented Feb 9, 2023

See #9.

Once merged, please patch version bump package.json from 1.0.3 to 1.0.4 and then release to npm.

If you could ping us back once done, that'd be great - then we can bump this https://github.com/posthtml/posthtml-inline-css/blob/master/package.json#L34 and the bump will be fixed in posthtml-inline-css too.

@phloe
Copy link
Collaborator

phloe commented Feb 9, 2023

Hmmm... It's not ideal that the match helper function throws like that - I agree - but you're feeding it a pseudo selector (i.e. :not()) which is not currently supported:

Screenshot 2023-02-10 at 00 17 31

The proposed fix would just make the match helper fail silently? - or not return the correct nodes?

@titanism
Copy link
Author

titanism commented Feb 9, 2023

It's hard to say since this package doesn't have tests - but I think it would just not return matches if they didn't exist?

@titanism
Copy link
Author

titanism commented Feb 9, 2023

The fix worked for me when manually implemented in my node_modules folder, so I think this might be best, unless you wrap that with a try/catch.

@phloe
Copy link
Collaborator

phloe commented Feb 9, 2023

Please read the documentation for supported features: https://github.com/posthtml/posthtml-match-helper

Only a subset of CSS selectors are supported.

@titanism
Copy link
Author

titanism commented Feb 9, 2023

The alternative is to wrap with a try/catch. I'll re-submit PR.

titanism added a commit to titanism/posthtml-match-helper that referenced this issue Feb 9, 2023
@titanism
Copy link
Author

titanism commented Feb 9, 2023

@phloe please see updated PR, this simply wraps with a try/catch, which respects the existing setup/rules/supported features.

titanism@24ca334

@phloe
Copy link
Collaborator

phloe commented Feb 10, 2023

I think the best strategy would be for posthtml-inline-css to strip pseudo selectors before handing them to the match helper - or wrap the block where it's used in try/catch.

Just wrapping in try/catch will mean selectors that would have matched are left out of the inlined css - negating what you are trying to achieve by inlining :(

Of course adding proper support for :not() (and maybe other pseudo selectors) might be doable - but would require some work.

@titanism
Copy link
Author

We can share a bug bounty reward for this over PayPal - or sponsor you on GitHub if you're interested in adding that!

@titanism
Copy link
Author

Actually - we're closing this as we don't need to inline CSS anymore.

A majority of clients now support it, so we're dropping inline CSS support.

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

No branches or pull requests

2 participants