-
Notifications
You must be signed in to change notification settings - Fork 25
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
Could we allow creating a no-imports stylesheet synchronously? #38
Comments
@tabatkins mentioned in #25 (comment) that large parses might be a problem. |
I just think there are so many hacks around it, that are easily encapsulated into libraries, that attempting to prevent authors from doing so will be futile, and just lead to them doing more buggy or expensive things instead. (E.g. my above, or inserting a |
Hmm, that's a valid concern, but if we do make a synchronous constructor, wouldn't that just take the same time as On the |
The issue with In particular I think if we decide in #34 to allow use of |
I still think adding a synchronously-created stylesheet is a good idea, but mostly if there’s some way to efficiently edit that stylesheet after its initial creation. This is somewhat similar in developer ergonomics as something like |
As an outsider this feels like you’re trying to stuff a square peg into a round hole. I’m starting to wonder if a new type of Just my two cents. Take it for whatever you will. |
That only works if the But creating an empty sheet and filling it rule-by-rule does indeed work, so that's annoying. :/ I'm not opposed to having a synchronous way to create a content-ful stylesheet. I am opposed to the sync way being easier and more natural to type than the async way. Perhaps a long-named method would work. ^_^ |
If we do go with the annoying-sync method, do we still not allow/ignore imports in that case? (Previous suggestion to not allow |
I guess it's better to allow it, it should be the author's responsibility to not place imports where they don't want it to block for too long. But I wonder what should happen if any of the imports fail. For the async version, we will reject the promise. Do we return null in this case if an import fail? |
So, thinking on this more, even the "rules can be parsed synchronously" doesn't really make things any better; as long as you're not using an @import, @charset, or similar, you can trivially just wrap the entire stylesheet text in an always-true @media rule and go to town. So! I think I'll change strategy for the CSS Parser API. Relevant to this repository, we should stop worrying about synchronous parsing as something particularly bad. So my proposal:
So the sync is slightly longer, which satisfies my "sync shouldn't be eaiser to type than async" requirement, but isn't too bad.
So, we can't sync-process @import; that would be equivalent to a sync-XHR, which is a big no-no. So we're got two choices:
The first one means we have to do something special for the So, I think we have to go with the second - if you do a sync parse, you don't get to use @import. If we add the sync version, we can eliminate |
Alternately, we allow them, but auto-fail the import, so they act the same as if you'd pointed them to a 404. |
Syntax error here means throwing an exception, or just dropping the rule? |
Sorry, throwing a |
I was talking about the "parse a stylesheet" entry point which currently never generates a syntax error. I'm just saying there's a third option of dropping |
Yeah, the grammar for CSS stylesheets is the regex For a similar example, using the string-OM, you can set a property to any value whatsoever; unknown/invalid values will just be silently ignored, just like they are when written into stylesheets. But Typed OM purposely breaks from this and throws a TypeError if it doesn't understand the value you're setting. This provides better feedback to the author, and JS code can respond to errors better than CSS can, so we can be more explicit with rejection. Thus, I think we should match this sort of behavior with newer JS parsing APIs - if we're not allowing some construct, we should throw an error to let the author know immediately, rather than just producing a stylesheet that acts differently than they thought it would. (I do not think we should fail the parsing just because we encounter something we don't recognize, like Typed OM does; the use-case here is parsing entire stylesheets, and stylesheets are intended to be written with forward-compatible error-recovery in mind. But @import is something we're explicitly disallowing, so it feels different to me.) |
Makes sense. |
A new factory function, createCSSStyleSheetSync will allow synchronous creation of CSSStyleSheet objects that have text but do not have @import rules. If the sheet text turn out to have @import rules, an exception will be thrown. Draft spec at: https://wicg.github.io/construct-stylesheets/ Discussion: WICG/construct-stylesheets#38 (comment) Bug: 807560 Change-Id: I9f8ef4151f616ecc343a12f3ddd0e5a31e1be363 Reviewed-on: https://chromium-review.googlesource.com/c/1237803 Commit-Queue: Rakina Zata Amni <[email protected]> Reviewed-by: Rune Lillesveen <[email protected]> Cr-Commit-Position: refs/heads/master@{#598731}
Spec updated at #39. |
It makes sense that
createCSSStyleSheet
has to be async to wait for@import
rules.Would it be possible to allow the creation of a no-import stylesheet synchronously, with a new factory method?
Otherwise I fear we are going to quickly see developers writing code like this:
which I'm sure is very buggy, but will mostly work.
The text was updated successfully, but these errors were encountered: