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

Add observers for stylesheet mutations #177

Merged
merged 9 commits into from
Feb 22, 2020

Conversation

dcramer
Copy link
Contributor

@dcramer dcramer commented Feb 21, 2020

This captures the insertRule and deleteRule actions on CSSStyleSheet, and wires them up to StyleSheetRule events in the recorder.

@dcramer
Copy link
Contributor Author

dcramer commented Feb 21, 2020

We should squash this for sure, but let me know if the approach is how you'd go about it.

@dcramer
Copy link
Contributor Author

dcramer commented Feb 21, 2020

Refs #104 #58
Fixes #176

@dcramer
Copy link
Contributor Author

dcramer commented Feb 21, 2020

cc @marcospassos

headless: false,
args: ['--no-sandbox'],
});
this.browser = await launchPuppeteer();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes are unrelated, but I was annoyed with running --watch and having the browser constantly take focus

@dcramer
Copy link
Contributor Author

dcramer commented Feb 21, 2020

Also looks like the test suite is a bit flakey (sometimes the async code doesnt finish running as expected?)

Have seen it on the new tests + on:

     AssertionError: Received value does not match stored snapshot "async-checkout 1".

@@ -4,6 +4,7 @@
"description": "record and replay the web",
"scripts": {
"test": "npm run bundle:browser && cross-env TS_NODE_CACHE=false TS_NODE_FILES=true mocha -r ts-node/register test/**/*.test.ts",
"test:watch": "PUPPETEER_HEADLESS=true npm run test -- --watch --watch-extensions js,ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can back this out if folks disagree but found it useful

aside, it doesnt recompile src/ and I couldn't figure out how to achieve that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not get that done too...
Combine Mocha and Typescript is not as fluent as jest.

src/record/observer.ts Show resolved Hide resolved
const insertRule = CSSStyleSheet.prototype.insertRule;
CSSStyleSheet.prototype.insertRule = function(
rule: string,
index?: number | undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like index?: number is enough since ? already make type optional

@@ -4,6 +4,7 @@
"description": "record and replay the web",
"scripts": {
"test": "npm run bundle:browser && cross-env TS_NODE_CACHE=false TS_NODE_FILES=true mocha -r ts-node/register test/**/*.test.ts",
"test:watch": "PUPPETEER_HEADLESS=true npm run test -- --watch --watch-extensions js,ts",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not get that done too...
Combine Mocha and Typescript is not as fluent as jest.

@Yuyz0112
Copy link
Member

@dcramer Thanks a lot! I would love to merge it after tweak some small changes.

The test things can be done in the following PRs.

@dcramer
Copy link
Contributor Author

dcramer commented Feb 22, 2020

@Yuyz0112 I fixed the type - were you asking me to pull the test:watch and other changes out?

@Yuyz0112
Copy link
Member

Nope, it's fine.

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

Successfully merging this pull request may close these issues.

3 participants