swap cssom with rrweb-cssom by seanparmelee · Pull Request #3497 · jsdom/jsdom

seanparmelee · 2023-01-12T20:35:31Z

I didn't see any feedback on the issue, but thought I'd go ahead and open a PR for swapping cssom with rrweb-cssom to address this warning we're seeing in our unit tests:

Warning: [JSS] CSSOM.parse is not a function

In short, there's a PR to fix this issue in cssom, but looks like that repo is no longer maintained. The rrweb fork contains this fix: NV/CSSOM@master...rrweb-io:CSSOM:master so we're hoping you're open to swapping the two.

Assuming you're OK with this change, I'd ultimately like for this to get into a 20.x release since that's the version jest-environment-jsdom is currently using.

domenic · 2023-01-13T01:53:00Z

Thanks for contributing! A maintained version of cssom would be very welcome.

To merge this, we'd need a test that fails before this change, and passes afterward.

We also do not maintain older major versions of jsdom, so you'd need to work with jest-environment-jsdom to get them updated if you wanted to use this indirectly.

seanparmelee · 2023-01-13T02:18:49Z

Thanks @domenic. As for the test, would you mind telling me what I'm missing. When I run yarn test, it fails because it cannot find the tuwpt-manifest.json file. Is that something I'm supposed to create manually or was that supposed to be generated by some other step? Thanks in advance!

Error: ENOENT: no such file or directory, open '/Users/sean/Projects/jsdom/test/web-platform-tests/tuwpt-manifest.json'
    at Object.openSync (node:fs:600:3)
    at Object.readFileSync (node:fs:468:35)
    at exports.readManifest (/Users/sean/Projects/jsdom/test/web-platform-tests/wpt-manifest-utils.js:44:29)
    at Object.<anonymous> (/Users/sean/Projects/jsdom/test/web-platform-tests/run-tuwpts.js:18:18)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)
    at Module._load (node:internal/modules/cjs/loader:878:12)
    at Module.require (node:internal/modules/cjs/loader:1061:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (/Users/sean/Projects/jsdom/test/index.js:67:1)
    at Module._compile (node:internal/modules/cjs/loader:1159:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1213:10)
    at Module.load (node:internal/modules/cjs/loader:1037:32)
    at Module._load (node:internal/modules/cjs/loader:878:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:169:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:438:15)
    at async formattedImport (/Users/sean/Projects/jsdom/node_modules/mocha/lib/nodejs/esm-utils.js:7:14)
    at async exports.requireOrImport (/Users/sean/Projects/jsdom/node_modules/mocha/lib/nodejs/esm-utils.js:38:28)
    at async exports.loadFilesAsync (/Users/sean/Projects/jsdom/node_modules/mocha/lib/nodejs/esm-utils.js:91:20)
    at async singleRun (/Users/sean/Projects/jsdom/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at async exports.handler (/Users/sean/Projects/jsdom/node_modules/mocha/lib/cli/run.js:370:5)
error Command failed with exit code 1.

domenic · 2023-01-13T02:34:41Z

Hmm. That file should be generated automatically as part of the command:

jsdom/test/web-platform-tests/run-tuwpts.js

Lines 15 to 16 in 79c351b

    
           const args = ["./wpt.py", "manifest", "--tests-root", testsRootArg, "--path", pathArg]; 
        
           spawnSync("python", args, { cwd: wptPath, stdio: "inherit" });

.

seanparmelee · 2023-01-13T05:45:39Z

The test/web-platform-tests/tests/css/cssom/CSSGroupingRule-insertRule.html file covers the functionality that this change affects. I tried adding DIR: css/cssom to the to-run.yaml file to see what would happen. The good news is that some of the CSSGroupingRule-insertRule tests are now passing. The bad news is that it's not all of them 😞 There's also quite a few css/cssom tests that are still failing so I don't think there's much benefit in going this route.

Shall I add a new test file in the to-upstream folder that's essentially CSSGroupingRule-insertRule.html, but only tests that are now passing? Or how would you prefer I proceed on this?

domenic · 2023-01-13T05:47:16Z

Yeah, that pattern works reasonably well and is one we've used in the past. We name the resulting files -dont-upstream.html to signal what's going on, and comment out the still-failing tests (ideally with an explanation of what's still broken). You can find a couple examples in the repo already.

seanparmelee · 2023-01-17T23:35:51Z

Hi @domenic, I've added the tests. Please let me know if this PR needs anything else.

domenic · 2023-01-17T23:36:49Z

I won't know until I have time to review, which usually happens on weekends, although not every weekend.

domenic

This looks good. However we should avoid having two versions of cssom, one indirectly via https://github.com/jsdom/cssstyle . That repo isn't maintained that well either (help welcome...) but I think I have permissions to push a new version with dependencies updated, at least...

This includes: * A replacement of cssom with rrweb-cssom, matching jsdom/jsdom#3497. Closes #153. * An update to the download_latest_properties.js script to stop using request. * A regeneration of the list of properties. Closes #150. * Some style updates to most generated files, for unclear reasons.

domenic · 2023-01-22T10:27:22Z

It turns out I have permission to edit cssstyle on GitHub, but not to publish to npm. I've emailed the people who do have such permissions asking for them to assign them over to me. But unfortunately that means there will be a bit of a delay before we can get this merged.

swap cssom with rrweb-cssom

f308436

seanparmelee force-pushed the rrweb-cssom branch from a3a9790 to f308436 Compare January 12, 2023 20:43

seanparmelee marked this pull request as ready for review January 12, 2023 21:27

add tests

08f8abe

domenic approved these changes Jan 22, 2023

View reviewed changes

domenic self-assigned this Jan 22, 2023

domenic mentioned this pull request Jan 22, 2023

Update dependencies and dev dependencies jsdom/cssstyle#160

Merged

seanparmelee and others added 2 commits January 27, 2023 11:24

Merge branch 'master' into rrweb-cssom

d04112e

Upgrade cssstyle too

2249099

domenic merged commit 5a4f77d into jsdom:master Feb 13, 2023

seanparmelee deleted the rrweb-cssom branch February 13, 2023 15:15

jantimon mentioned this pull request May 2, 2024

Using @container query logs error #3597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

swap cssom with rrweb-cssom#3497

swap cssom with rrweb-cssom#3497
domenic merged 4 commits intojsdom:masterfrom
seanparmelee:rrweb-cssom

seanparmelee commented Jan 12, 2023 •

edited

Loading

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 13, 2023

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 13, 2023

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 17, 2023

Uh oh!

domenic commented Jan 17, 2023

Uh oh!

domenic left a comment

Uh oh!

domenic commented Jan 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

seanparmelee commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 13, 2023

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 13, 2023

Uh oh!

domenic commented Jan 13, 2023

Uh oh!

seanparmelee commented Jan 17, 2023

Uh oh!

domenic commented Jan 17, 2023

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

domenic commented Jan 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

seanparmelee commented Jan 12, 2023 •

edited

Loading