-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Migrate CSSStyleDeclaration
to WebIDL2JS
#116
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Can't comment on the CSS parts too much, but the Web IDL stuff looks solid. I do of course defer to @jsakas for a full review.
Codecov Report
@@ Coverage Diff @@
## master #116 +/- ##
==========================================
- Coverage 37.39% 36.01% -1.38%
==========================================
Files 87 86 -1
Lines 1182 1155 -27
Branches 227 229 +2
==========================================
- Hits 442 416 -26
+ Misses 633 632 -1
Partials 107 107
Continue to review full report at Codecov.
|
56a48b5
to
c3d2de8
Compare
The build passed, but it’s not reflected on GitHub due to |
travis-ci.com seems to have been down for maintenance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also write some documentation on the new setup? Two things in particular: the webidl2js-wrapper
, and the fact that the default CSSStyleDeclaration
export takes a parameter (but not the one exposed by webidl2js-wrapper
).
Also an interesting question would be do we want to add some custom reflection support for CSS attributes? I think it might increase memory usage, but would allow us to get rid of the getter/setter methods on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as I’m concerned. I’ll leave @jsakas to do a final pass and merge however.
e5272ba
to
7156c3f
Compare
Thanks guys for all your work here. Diving into this PR this week. |
e46e859
to
0f13723
Compare
5486c46
to
d8f8f98
Compare
Co-authored-by: Timothy Gu <[email protected]>
d8f8f98
to
c6b72e0
Compare
Hello, I created #140 before truly understanding this morning what this PR was about. The initial goal of my PR was to convert values passed to an interface of CSSStyleDeclaration into CSSOMString, when that interface expected such type. But this PR already handles that. My PR fixes many other issues and implements support for several CSS properties and value types. So I allowed myself to rebase my PR on this one. I only kept my tests related to the conversion to CSSOMString. I would like to add a few notes here, some of which I have already made in other places. 1/ I'm not sure I understand why 2/ The way CSS values are parsed in this library does not conform to the CSSOM specification and especially the CSS Syntax specification, ie. a "global" parsing step rather than lexing (tokenizing), consuming tokens for parsing, and serializing tokens. A "global" parse/serialize strategy is easier to implement and perhaps more efficient, but it may be preferable and even necessary to conform to those specifications before being forced to a large rewrite. This would also make it possible to avoid using the "higher order setter" mentioned in the first point. 3/ This PR is open since more than 1 year. Would it be possible to have this PR and/or my PR reviewed by someone else? AS noted above, it fixes many |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 37.39% 36.01% -1.38%
==========================================
Files 87 86 -1
Lines 1182 1155 -27
Branches 227 229 +2
==========================================
- Hits 442 416 -26
+ Misses 633 632 -1
Partials 107 107 ☔ View full report in Codecov by Sentry. |
This migrates
CSSStyleDeclaration
to use WebIDL2JS.Part of jsdom/jsdom#2727.
There’s also jsdom/webidl2js#188, which would simplify the generation of
CSSStyleDeclaration‑properties.webidl
by allowing the resulting string to be passed to WebIDL2JS directly, instead of having to write it to a temporary file.review?(@jsakas, @TimothyGu)