Report invalid selectors #336
Conversation
✅ Deploy Preview for anchor-position-wpt canceled.
|
✅ Deploy Preview for anchor-polyfill ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
I'm okay merging this, but it feels like it might be better to more generally expose parsing errors from CSSTree rather than catching this one specific selector case. I think we could use onParseError in our single call to CSSTree parse, e.g.:
onParseError: (err) => {
throw new Error(err.formattedMessage);
},Or if we don't want to stop execution, in case the invalid CSS doesn't require the polyfill:
onParseError: (err) => console.error(err.formattedMessage),@jamesnw What do you think?
47295b9 to
117e2bb
Compare
|
I think you're right that a more global option would be helpful. I'm still leaning towards throwing an error and stopping execution. Catching the error in I updated this PR with this. I included the original error as the cause, and also added a note to make it clear that the polyfill did not complete. |
Right, but sometimes the invalid CSS doesn't contain any relevant (to anchor positioning) CSS, and the polyfill still executes perfectly fine. For example, this triggers .foo {
anchor-name: var(w-[10]);
}While this triggers .foo {
width: var(w-[10]);
}I don't think we want the polyfill to throw on just any CSS parsing error, but I do think we want to expose parsing errors to users to help with debugging. |
|
Hmm. I don't think the polyfill should take the role of a linter, and it could be confusing if the polyfill is reporting CSS parse errors that are unrelated to the polyfill working. It also is confusing, as you point out, for the polyfill to not work if there are unrelated CSS parse errors that don't break the polyfill. In the ideal situation, the polyfill would only surface parse errors if it results in the polyfill not running. There are two ways of doing that- first would be identifying parse errors that do break the polyfill, and catch and throw those. But as you point out, that's going to be pretty context specific. The other way would be storing all the parse errors, and then wrapping the entire polyfill in a try catch. If it throws, then reporting the parse errors. I don't like that idea either. My current leaning is going back to the original solution- catching individual errors in places where they will break the polyfill, and throwing with better guidance on the fix. Thoughts? |
I guess this approach doesn't really bother me, and it seems much more straightforward than trying to anticipate places in our code where malformed CSS (resulting in an unparsed I don't feel strongly though, so if you think logging from |
|
@jgerigmeyer I haven't fixed the tests yet, but I'm curious on your take on this. I think I have it reporting only parse errors in the user's stylesheets, and only when the polyfill fails. I store all the parse errors during the cascade step, since that's the initial pass through the CSS. Otherwise, we'd be reporting on errors parsing stylesheets that were changed by the polyfill. I only wrapped the cascade and parse steps in the try catch, as they are the steps that parse CSS. While it's possible that a syntax error would cause the polyfill to fail in later steps, I think it's more likely that the syntax error would be unrelated, and likely to be a red herring. It does report all parse errors, and doesn't attempt to identify which parse error caused the polyfill to fail. |
Description
Invalid selectors, for instance
.w-[10], crash the polyfill. This PR throws a more specific error to guide the developer in fixing it. It does not attempt to ignore or fix the error.There may be other places where invalid CSS will break the polyfill's assumption of validity, and we could use a similar approach as they are reported.
Related Issue(s)
Closes #335
Steps to test/reproduce
Locally, add a rule with a selector like
.w-[10]that is invalid, and see that the error is thrown.Or, view console at https://codepen.io/jamessw/pen/GgJdEeM