Skip to content

Conversation

@AlfieJones
Copy link
Contributor

In the rrweb-snapshot package, there are a few references to a global document object instead of using the doc parameter.
This pr moves them to the doc parameter inline with the rest of the package.

There's room for improving the typescript support to throw an error when accessing the document element but I wasn't able to figure out a clean way to achieve this.

@changeset-bot
Copy link

changeset-bot bot commented May 22, 2024

🦋 Changeset detected

Latest commit: 6869ab4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb-snapshot Patch
rrweb Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eoghanmurray
Copy link
Contributor

Hi Alfie, thanks for your PR!

Can you explain if there was a situation where the use of the global document failed here, or is it just for completeness?
If it can fail maybe I can help you create a test for the situation.

@AlfieJones
Copy link
Contributor Author

I was running the rebuild outside of a dom environment using jsdom so any references to the global document would fail.

@eoghanmurray
Copy link
Contributor

Thank you.
I've added a cursory test to ensure we don't get a regression in the main code paths.
The one you fixed though was related to canvas and I couldn't recreate a failure using the jsdom in the tests without more effort.
Feel free to add to that test if you wish it to prevent regressions in that sense, but overall it's probably not necessary.
If you think it's worthwhile to share what you tried in terms of typescript enforcement, I'd be interested to see how that might work.

@eoghanmurray
Copy link
Contributor

I also explored a test on the rebuild side, but don't think it's worth adding;
https://gist.github.com/eoghanmurray/52d0730eb8d7a209e27f29de0ddde455

Update patch text with my best understanding
@eoghanmurray eoghanmurray merged commit f3cf092 into rrweb-io:master Jun 4, 2024
jeffdnguyen pushed a commit to pendo-io/rrweb that referenced this pull request Jul 29, 2024
fix for options `recordCanvas: true`: 
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment
jeffdnguyen pushed a commit to pendo-io/rrweb that referenced this pull request Jul 29, 2024
fix for options `recordCanvas: true`: 
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Sep 17, 2024
fix for options `recordCanvas: true`:
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment
jxiwang pushed a commit to amplitude/rrweb that referenced this pull request Oct 17, 2024
fix for options `recordCanvas: true`: 
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment
jxiwang pushed a commit to amplitude/rrweb that referenced this pull request Oct 17, 2024
fix for options `recordCanvas: true`: 
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment
kanishq1 added a commit to devrev/rrweb that referenced this pull request Mar 27, 2025
* Removing global document references (rrweb-io#1482)

fix for options `recordCanvas: true`: 
* replace document.createElement with doc.createElement in rrweb-snapshot code
* Eoghan: add a regression test to prevent future accidental use of `document` instead of `doc`.  This test can be excised if a new feature can only be run in the browser and not in the jsdom environment

* No neg lookbehind (rrweb-io#1493)

* Older versions of Safari 16 don't support lookbehind assertions - https://caniuse.com/js-regexp-lookbehind

* Refactor to show the similarity between these two regexes

* Apply formatting changes

* Create no-neg-lookbehind.md

---------

Co-authored-by: eoghanmurray <[email protected]>

* Refactor to preclude the need for a continuous raf loop (rrweb-io#1328)

* Refactor to preclude the need for a continuous raf loop

* Apply formatting changes

* Create shadow-dom-unbusify.md

---------

Co-authored-by: eoghanmurray <[email protected]>

* perf(mutation): refactor parent removed detection to iterative procedure (rrweb-io#1489)

* perf(mutation): add deep tree benchmark

* perf(mutation): use iterative procedure

* perf(mutation): run formatter

* perf(mutation): add changeset

* Chore: Migrate build to vite (rrweb-io#1033)

* Chore: Add move most types from rrweb to @rrweb/types package

* Split off type imports

* Split off type import to its own line

* Get vite to generate type definitions

* Apply formatting changes

* noEmit not allowed in tsconfig, moved it to build step

* Migrate rrdom-nodejs build to vite

* Apply formatting changes

* Migrate rrweb-snapshot to vite

* Unify configs

* Chore: Migrate rrdom to vite

Turns out what we where doing by overwriting `public textContent: string | undefined` as a getter in a subclass is something that isn't allowed in typescript. Because we where using `// @ts-ignore`  to hide this error our bundler chose to allow the overwrite. Vite choses to disallow the overwrite making all subclasses' `textContent` undefined.
To mitigate this we're using an abstract class, which does allow sub classes to decide if they wan't to use getters or not.

* Chore: Migrate rrweb to vite WIP

* build:browser was removed (for now)

* BREAKING: moved rrweb-plugin-console to its own npm module

This removes console from rrweb-all.js

* Support cjs files in startServer

* Move canvas-webrtc plugin to its own package

* Chore: move sequential-id plugin to its own package

* Chore: Configure rrweb's vite bundling

* `Id` had lowercase `d` before, making it lowercase again

* Test: Move console tests to their own package

* remove unused utils from rrdom

* pull in latest version of master
something when wrong earlier when resolving merge conflicts, this should be correct

* Fix type casting issue in diff.ts

* Fix typo

* Fix duplicate entries in package.json and tsconfig.json

* Apply formatting changes

* Update dependencies in package.json files

* Update dependencies to use Vite 5.2.8 in package.json files

* Get tests passing for rrdom

`apply virtual style rules to node` tests need to be moved to rrweb to avoid circular dependencies

* Fix image loading issue in integration tests

* Move pack/unpack to its own @rrweb/packer module

* Get tests to work in rrdom-nodejs

* Port tests in rrweb-snapshot to vitest and fix them

* Fix tests for rrweb-plugin-console-record

* Add @rrweb/all package

* Fix publint and attw errors for rrdom and @rrweb/types

* Use shared vitest.config.ts in rrweb-snapshot package

* Fix publint and attw issues for rrweb-snapshot

* Export `ReplayPlugin` type directly from rrweb

* Fix publint and attw issues for packages

* Fix publint & attw issue.

I was bumping into this issue: https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/3729bc2a3ca2ef7dda5c22fef81f89e1abe5dacf/docs/problems/NoResolution.md

And had to choose one of these three methods described here:
https://github.com/andrewbranch/example-subpath-exports-ts-compat?tab=readme-ov-file#typescript-friendly-strategies-for-packagejson-subpath-exports-compatibility
And I ended up going for the method described here:
https://github.com/andrewbranch/example-subpath-exports-ts-compat/tree/1ffe3425b0a7ad8ecdf3c373f76f431ee341366b/examples/node_modules/package-json-redirects#package-json-redirects

The redirect method seemed the least invasive and most effective.

* Fix publint & attw issue.

I was bumping into this issue: https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/3729bc2a3ca2ef7dda5c22fef81f89e1abe5dacf/docs/problems/NoResolution.md

And had to choose one of these three methods described here:
https://github.com/andrewbranch/example-subpath-exports-ts-compat?tab=readme-ov-file#typescript-friendly-strategies-for-packagejson-subpath-exports-compatibility
And I ended up going for the method described here:
https://github.com/andrewbranch/example-subpath-exports-ts-compat/tree/1ffe3425b0a7ad8ecdf3c373f76f431ee341366b/examples/node_modules/package-json-redirects#package-json-redirects

The redirect method seemed the least invasive and most effective.

* move some rrdom tests that require rrweb to rrweb package

* Use pre-jest 29 syntax for snapshotting

* get rrweb passing publint and attw

* const enum does not work with isolated modules flag

* Fix script tag type in webgl.test.ts.snap and update rrweb.umd.cjs path in webgl.test.ts

* Fix paths

* Move tests for console record plugin and fix bundle path

* Fix tests for rrweb

* pack integration tests were moved to @rrweb/all

* Update rrweb bundle path in test files

* Fix flaky scroll emit from test

* Migrate rrweb's tests over to vitest and make them pass

* Make sure benchmarks & updating tests work

* Remove jest from rrweb

* Fix paths

* always use rrweb's own cssom

* Update tsconfig.json for rrweb-plugin-sequential-id-record

Fixes this error:
Error: @rrweb/rrweb-plugin-sequential-id-record:prepublish: tsconfig.json(9,5): error TS6377: Cannot write file '/home/runner/work/rrweb/rrweb/tsconfig.tsbuildinfo' because it will overwrite '.tsbuildinfo' file generated by referenced project '/home/runner/work/rrweb/rrweb/packages/rrweb'

* Add tsbuildinfo config to extended tsconfig files

* Move rrdom over to vitest

* Apply formatting changes

* Update rrweb imports to use the new package structure

* extend rrweb-snapshot's tsconfig from monorepo base config

* extend @rrweb/types's tsconfig from monorepo base config

* extend rrdom's tsconfig from monorepo base config

* extend rrdom-nodejs's tsconfig from monorepo base config

* extend web-extension's tsconfig from monorepo base config

* unify tsconfigs

* Continue when tests fail

* Add stricter type checking

* Add check-types global command

* remove jest

* Remove unused code

* Add check-types command to build script

* Fix linting issues

* Add setup Chrome action for CI/CD workflow

* Update puppeteer version in package.json for rrweb

* Update Chrome setup in CI/CD workflow

* Update Chrome setup in CI/CD workflow

* Add Chrome setup and test cache location

* Update CI/CD workflow to test chrome cache location

* Add chrome installation step to CI/CD workflow

* Update Puppeteer configuration for headless testing

* Update dependencies and workflow configuration

* Use same version of chrome on CI as is run locally

* Use version of chrome that seems to work with rrdom tests

* Try using puppeteerrc to define chrome version

* Add .cache directory to .gitignore

* Move global flag to vitest config

* Update puppeteer version to 20.9.0

* Update console log messages in rrweb-plugin-console-record for new puppeteer version

* Remove redundant Chrome setup from CI/CD workflow

* Add minification and umd for all built files

* Update import paths for rrweb dist files

* Add @rrweb/replay and @rrweb/record

* Add script to lint packages

* Apply formatting changes

* exclude styles export from typescript package type checking

* WIP Move rrweb-player over to vite

* Apply formatting changes

* chore: Update rrweb plugin import paths

* Remove rollup from rrweb-player

* Fix typing issues

* Fix typing issues

* chore: Update rrweb-player to use vite for build process

* Apply formatting changes

* chore: Export Player class in rrweb-player/src/main.ts

Makes attw happy

* Apply formatting changes

* Gets wiped by yarn workspaces-to-typescript-project-references

* Add .eslintignore and .eslintrc.cjs files for rrweb-player package

* Apply formatting changes

* Update dependencies in rrweb-player/package.json

* Apply formatting changes

* chore: Update eslint configuration for rrweb-player package

* Apply formatting changes

* chore: Remove unused files from rrweb-player package

* Apply formatting changes

* chore: Update rrweb-player import path to use rrweb-player.cjs

* chore: Update addEventListener signature in rrweb-player

* Apply formatting changes

* Add .eslintignore and update .gitignore files for to root

* Apply formatting changes

* Update documentation

* Update @rrweb/types package description

* Apply formatting changes

* Update build and run commands in CONTRIBUTING.md

* Apply formatting changes

* Update package versions to 2.0.0-alpha.13

* Apply formatting changes

* Apply formatting changes

* Fix import statement in media/index.ts

* Apply formatting changes

* chore: Update .gitignore to exclude build and dist directories

* Apply formatting changes

* Apply formatting changes

* Migrate setTimeout to vitest

* Apply formatting changes

* Apply formatting changes

* Fix isNativeShadowDom function signature in utils.ts

* try out jsr

* Apply formatting changes

* Update package versions to 2.0.0-alpha.14

* Apply formatting changes

* Fix name of rrwebSnapshot object

* Apply formatting changes

* Remove unused lock files

* Apply formatting changes

* Update rrweb bundle path to use umd.cjs format

* Apply formatting changes

* Trigger tests to run again

* Rename snapshots for vitest

* Apply formatting changes

* Ping CI

* Apply formatting changes

* Ping CI

* Apply formatting changes

* Ignore files generated by svelte-kit for prettier

* Correct Player object

* Apply formatting changes

---------

Co-authored-by: Alfie Jones <[email protected]>
Co-authored-by: Eoghan Murray <[email protected]>
Co-authored-by: eoghanmurray <[email protected]>
Co-authored-by: Jonas <[email protected]>
Co-authored-by: Justin Halsall <[email protected]>
Co-authored-by: kanishq1 <[email protected]>
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.

2 participants