Skip to content

feat(core)!: Remove getDomElement method #14797

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

Merged
merged 3 commits into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/migration/v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ It will be removed in a future major version.
- The `BAGGAGE_HEADER_NAME` export has been removed. Use `"baggage"` string constant directly instead.
- The `flatten` export has been removed. There is no replacement.
- The `urlEncode` method has been removed. There is no replacement.
- The `getDomElement` method has been removed. There is no replacement.

### `@sentry/nestjs`

Expand Down
35 changes: 24 additions & 11 deletions packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
getActiveSpan,
getClient,
getCurrentScope,
getDomElement,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getRootSpan,
Expand Down Expand Up @@ -190,6 +189,12 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
* We explicitly export the proper type here, as this has to be extended in some cases.
*/
export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptions> = {}) => {
/**
* This is just a small wrapper that makes `document` optional.
* We want to be extra-safe and always check that this exists, to ensure weird environments do not blow up.
*/
const optionalWindowDocument = WINDOW.document as (typeof WINDOW)['document'] | undefined;

registerSpanErrorInstrumentation();

const {
Expand Down Expand Up @@ -273,13 +278,13 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
});

function emitFinish(): void {
if (['interactive', 'complete'].includes(WINDOW.document.readyState)) {
if (optionalWindowDocument && ['interactive', 'complete'].includes(optionalWindowDocument.readyState)) {
client.emit('idleSpanEnableAutoFinish', idleSpan);
}
}

if (isPageloadTransaction && WINDOW.document) {
WINDOW.document.addEventListener('readystatechange', () => {
if (isPageloadTransaction && optionalWindowDocument) {
optionalWindowDocument.addEventListener('readystatechange', () => {
emitFinish();
});

Expand Down Expand Up @@ -462,12 +467,14 @@ export function startBrowserTracingNavigationSpan(client: Client, spanOptions: S

/** Returns the value of a meta tag */
export function getMetaContent(metaName: string): string | undefined {
// Can't specify generic to `getDomElement` because tracing can be used
// in a variety of environments, have to disable `no-unsafe-member-access`
// as a result.
const metaTag = getDomElement(`meta[name=${metaName}]`);
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return metaTag ? metaTag.getAttribute('content') : undefined;
/**
* This is just a small wrapper that makes `document` optional.
* We want to be extra-safe and always check that this exists, to ensure weird environments do not blow up.
*/
const optionalWindowDocument = WINDOW.document as (typeof WINDOW)['document'] | undefined;

const metaTag = optionalWindowDocument && optionalWindowDocument.querySelector(`meta[name=${metaName}]`);
return (metaTag && metaTag.getAttribute('content')) || undefined;
}

/** Start listener for interaction transactions */
Expand All @@ -477,6 +484,12 @@ function registerInteractionListener(
childSpanTimeout: BrowserTracingOptions['childSpanTimeout'],
latestRoute: RouteInfo,
): void {
/**
* This is just a small wrapper that makes `document` optional.
* We want to be extra-safe and always check that this exists, to ensure weird environments do not blow up.
*/
const optionalWindowDocument = WINDOW.document as (typeof WINDOW)['document'] | undefined;

let inflightInteractionSpan: Span | undefined;
const registerInteractionTransaction = (): void => {
const op = 'ui.action.click';
Expand Down Expand Up @@ -519,7 +532,7 @@ function registerInteractionListener(
);
};

if (WINDOW.document) {
if (optionalWindowDocument) {
addEventListener('click', registerInteractionTransaction, { once: false, capture: true });
}
}
24 changes: 0 additions & 24 deletions packages/core/src/utils-hoist/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,30 +140,6 @@ export function getLocationHref(): string {
}
}

/**
* Gets a DOM element by using document.querySelector.
*
* This wrapper will first check for the existence of the function before
* actually calling it so that we don't have to take care of this check,
* every time we want to access the DOM.
*
* Reason: DOM/querySelector is not available in all environments.
*
* We have to cast to any because utils can be consumed by a variety of environments,
* and we don't want to break TS users. If you know what element will be selected by
* `document.querySelector`, specify it as part of the generic call. For example,
* `const element = getDomElement<Element>('selector');`
*
* @param selector the selector string passed on to document.querySelector
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function getDomElement<E = any>(selector: string): E | null {
if (WINDOW.document && WINDOW.document.querySelector) {
return WINDOW.document.querySelector(selector) as unknown as E;
}
return null;
}

/**
* Given a DOM element, traverses up the tree until it finds the first ancestor node
* that has the `data-sentry-component` or `data-sentry-element` attribute with `data-sentry-component` taking
Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/utils-hoist/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
export { applyAggregateErrorsToEvent } from './aggregate-errors';
export { getBreadcrumbLogLevelFromHttpStatusCode } from './breadcrumb-log-level';
export { getComponentName, getDomElement, getLocationHref, htmlTreeAsString } from './browser';
export {
getComponentName,
getLocationHref,
htmlTreeAsString,
} from './browser';
export { dsnFromString, dsnToString, makeDsn } from './dsn';
export { SentryError } from './error';
export { GLOBAL_OBJ } from './worldwide';
Expand Down
12 changes: 1 addition & 11 deletions packages/core/test/utils-hoist/browser.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { JSDOM } from 'jsdom';

import { getDomElement, htmlTreeAsString } from '../../src/utils-hoist/browser';
import { htmlTreeAsString } from '../../src/utils-hoist/browser';

beforeAll(() => {
const dom = new JSDOM();
Expand Down Expand Up @@ -74,13 +74,3 @@ describe('htmlTreeAsString', () => {
);
});
});

describe('getDomElement', () => {
it('returns the element for a given query selector', () => {
document.head.innerHTML = '<div id="mydiv">Hello</div>';
const el = getDomElement('div#mydiv');
expect(el).toBeDefined();
expect(el?.tagName).toEqual('DIV');
expect(el?.id).toEqual('mydiv');
});
});
5 changes: 3 additions & 2 deletions packages/svelte/src/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import type { BrowserOptions } from '@sentry/browser';
import { WINDOW } from '@sentry/browser';
import { addEventProcessor, init as browserInit } from '@sentry/browser';
import type { Client, EventProcessor } from '@sentry/core';
import { applySdkMetadata, getDomElement } from '@sentry/core';
import { applySdkMetadata } from '@sentry/core';
/**
* Inits the Svelte SDK
*/
Expand Down Expand Up @@ -55,5 +56,5 @@ export function detectAndReportSvelteKit(): void {
* @see https://github.com/sveltejs/kit/issues/307 for more information
*/
export function isSvelteKitApp(): boolean {
return getDomElement('div#svelte-announcer') !== null;
return !!WINDOW.document.querySelector('div#svelte-announcer');
}
4 changes: 0 additions & 4 deletions packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ import {
getBreadcrumbLogLevelFromHttpStatusCode as getBreadcrumbLogLevelFromHttpStatusCode_imported,
getComponentName as getComponentName_imported,
getDebugImagesForResources as getDebugImagesForResources_imported,
getDomElement as getDomElement_imported,
getEventDescription as getEventDescription_imported,
getFilenameToDebugIdMap as getFilenameToDebugIdMap_imported,
getFramesFromEvent as getFramesFromEvent_imported,
Expand Down Expand Up @@ -542,9 +541,6 @@ export const resolve = resolve_imported;
/** @deprecated Import from `@sentry/core` instead. */
export const getComponentName = getComponentName_imported;

/** @deprecated Import from `@sentry/core` instead. */
export const getDomElement = getDomElement_imported;

/** @deprecated Import from `@sentry/core` instead. */
export const getLocationHref = getLocationHref_imported;

Expand Down
Loading