8000 Merge branch 'develop' into priscila/feat/add-remix-e2e-test · Issawat/sentry-javascript@befe6fa · GitHub
[go: up one dir, main page]
Skip to content

Commit befe6fa

Browse files
priscilawebdevpriscilawebdev
authored
Merge branch 'develop' into priscila/feat/add-remix-e2e-test
2 parents 8cdc8a8 + 0008d94 commit befe6fa

File tree

14 files changed

+521
-148
lines changed

14 files changed

+521
-148
lines changed

‎.github/workflows/build.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,7 @@ jobs:
738738
github.actor != 'dependabot[bot]'
739739
needs: [job_get_metadata, job_build]
740740
runs-on: ubuntu-20.04
741-
timeout-minutes: 20
741+
timeout-minutes: 30
742742
strategy:
743743
fail-fast: false
744744
matrix:

‎LICENSE

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,21 @@
1-
Copyright (c) 2018 Sentry (https://sentry.io) and individual contributors. All rights reserved.
1+
MIT License
22

3-
Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated
4-
documentation files (the "Software"), to deal in the Software without restriction, including without limitation the
5-
rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit
6-
persons to whom the Software is furnished to do so, subject to the following conditions:
3+
Copyright (c) 2022 Functional Software, Inc. dba Sentry
74

8-
The above copyright notice and this permission notice shall be included in all copies or substantial portions of the
9-
Software.
5+
Permission is hereby granted, free of charge, to any person obtaining a copy of
6+
this software and associated documentation files (the "Software"), to deal in
7+
the Software without restriction, including without limitation the rights to
8+
use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
9+
of the Software, and to permit persons to whom the Software is furnished to do
10+
so, subject to the following conditions:
1011

11-
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
12-
WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
13-
COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
14-
OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
12+
The above copyright notice and this permission notice shall be included in all
13+
copies or substantial portions of the Software.
14+
15+
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
16+
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
17+
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
18+
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
19+
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
20+
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
21+
SOFTWARE.

‎packages/angular/tsconfig.json

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@
55

66
"compilerOptions": {
77
// package-specific options
8-
"experimentalDecorators": true
8+
"experimentalDecorators": true,
9+
// Avoid loading all @types/... packages as they may not be TS 4.0 compatible
10+
// We have no types packages here we directly depend on,
11+
// if we ever add some we need to allowlist them here
12+
"types": []
913
}
1014
}

‎packages/remix/src/utils/instrumentServer.ts

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -236,18 +236,26 @@ function makeWrappedRootLoader(origLoader: DataFunction): DataFunction {
236236
};
237237
}
238238

239-
// Note: `redirect` and `catch` responses do not have bodies to extract
240-
if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) {
241-
const data = await extractData(res);
242-
243-
if (typeof data === 'object') {
244-
return json(
245-
{ ...data, ...traceAndBaggage },
246-
{ headers: res.headers, statusText: res.statusText, status: res.status },
247-
);
248-
} else {
249-
__DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response body is not an object');
239+
if (isResponse(res)) {
240+
// Note: `redirect` and `catch` responses do not have bodies to extract.
241+
// We skip injection of trace and baggage in those cases.
242+
// For `redirect`, a valid internal redirection target will have the trace and baggage injected.
243+
if (isRedirectResponse(res) || isCatchResponse(res)) {
244+
__DEBUG_BUILD__ && logger.warn('Skipping injection of trace and baggage as the response does not have a body');
250245
return res;
246+
} else {
247+
const data = await extractData(res);
248+
249+
if (typeof data === 'object') {
250+
return json(
251+
{ ...data, ...traceAndBaggage },
252+
{ headers: res.headers, statusText: res.statusText, status: res.status },
253+
);
254+
} else {
255+
__DEBUG_BUILD__ &&
256+
logger.warn('Skipping injection of trace and baggage as the response body is not an object');
257+
return res;
258+
}
251259
}
252260
}
253261

‎packages/remix/test/integration/a 10000 pp_v1/root.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ export const loader: LoaderFunction = async ({ request }) => {
3434
throw redirect('/?type=plain');
3535
case 'returnRedirect':
3636
return redirect('/?type=plain');
37+
case 'throwRedirectToExternal':
38+
throw redirect('https://example.com');
39+
case 'returnRedirectToExternal':
40+
return redirect('https://example.com');
3741
default: {
3842
return {};
3943
}

‎packages/remix/test/integration/app_v2/root.tsx

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ export const loader: LoaderFunction = async ({ request }) => {
3434
throw redirect('/?type=plain');
3535
case 'returnRedirect':
3636
return redirect('/?type=plain');
37+
case 'throwRedirectToExternal':
38+
throw redirect('https://example.com');
39+
case 'returnRedirectToExternal':
40+
return redirect('https://example.com');
3741
default: {
3842
return {};
3943
}

‎packages/remix/test/integration/test/client/root-loader.test.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red
125125
}) => {
126126
await page.goto('/?type=throwRedirect');
127127

128+
// We should be successfully redirected to the path.
129+
expect(page.url()).toEqual(expect.stringContaining('/?type=plain'));
130+
128131
const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
129132

130133
expect(sentryTrace).toEqual(expect.any(String));
@@ -138,11 +141,14 @@ test('should inject `sentry-trace` and `baggage` into root loader throwing a red
138141
});
139142
});
140143

141-
test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to a plain object', async ({
144+
test('should inject `sentry-trace` and `baggage` into root loader returning a redirection to valid path.', async ({
142145
page,
143146
}) => {
144147
await page.goto('/?type=returnRedirect');
145148

149+
// We should be successfully redirected to the path.
150+
expect(page.url()).toEqual(expect.stringContaining('/?type=plain'));
151+
146152
const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
147153

148154
expect(sentryTrace).toEqual(expect.any(String));
@@ -155,3 +161,27 @@ test('should inject `sentry-trace` and `baggage` into root loader returning a re
155161
sentryBaggage: sentryBaggage,
156162
});
157163
});
164+
165+
test('should return redirect to an external path with no baggage and trace injected.', async ({ page }) => {
166+
await page.goto('/?type=returnRedirectToExternal');
167+
168+
// We should be successfully redirected to the external path.
169+
expect(page.url()).toEqual(expect.stringContaining('https://example.com'));
170+
171+
const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
172+
173+
expect(sentryTrace).toBeUndefined();
174+
expect(sentryBaggage).toBeUndefined();
175+
});
176+
177+
test('should throw redirect to an external path with no baggage and trace injected.', async ({ page }) => {
178+
await page.goto('/?type=throwRedirectToExternal');
179+
180+
// We should be successfully redirected to the external path.
181+
expect(page.url()).toEqual(expect.stringContaining('https://example.com'));
182+
183+
const { sentryTrace, sentryBaggage } = await extractTraceAndBaggageFromMeta(page);
184+
185+
expect(sentryTrace).toBeUndefined();
186+
expect(sentryBaggage).toBeUndefined();
187+
});

‎packages/sveltekit/src/client/load.ts

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
129129
return originalFetch;
130130
}
131131

132+
const options = client.getOptions();
133+
132134
const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined;
133135
const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined;
134136

@@ -147,7 +149,10 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
147149
const shouldAttachHeaders: (url: string) => boolean = url => {
148150
return (
149151
!!shouldTraceFetch &&
150-
stringMatchesSomePattern(url, browserTracingOptions.tracePropagationTargets || ['localhost', /^\//])
152+
stringMatchesSomePattern(
153+
url,
154+
options.tracePropagationTargets || browserTracingOptions.tracePropagationTargets || ['localhost', /^\//],
155+
)
151156
);
152157
};
153158

@@ -177,20 +182,15 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
177182
};
178183

179184
const patchedInit: RequestInit = { ...init };
180-
const activeSpan = getCurrentHub().getScope().getSpan();
181-
const activeTransaction = activeSpan && activeSpan.transaction;
182-
183-
const createSpan = activeTransaction && shouldCreateSpan(rawUrl);
184-
const attachHeaders = createSpan && activeTransaction && shouldAttachHeaders(rawUrl);
185-
186-
// only attach headers if we should create a span
187-
if (attachHeaders) {
188-
const dsc = activeTransaction.getDynamicSamplingContext();
185+
const hub = getCurrentHub();
186+
const scope = hub.getScope();
187+
const client = hub.getClient();
189188

189+
if (client && shouldAttachHeaders(rawUrl)) {
190190
const headers = addTracingHeadersToFetchRequest(
191191
input as string | Request,
192-
dsc,
193-
activeSpan,
192+
client,
193+
scope,
194194
patchedInit as {
195195
headers:
196196
| {
@@ -207,7 +207,7 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch
207207

208208
const patchedFetchArgs = [input, patchedInit];
209209

210-
if (createSpan) {
210+
if (shouldCreateSpan(rawUrl)) {
211211
fetchPromise = trace(
212212
{
213213
name: `${method} ${requestData.url}`, // this will become the description of the span

‎packages/sveltekit/test/client/load.test.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ const mockedGetIntegrationById = vi.fn(id => {
6161
const mockedGetClient = vi.fn(() => {
6262
return {
6363
getIntegrationById: mockedGetIntegrationById,
64+
getOptions: () => ({}),
6465
};
6566
});
6667

@@ -77,6 +78,11 @@ vi.mock('@sentry/core', async () => {
7778
getClient: mockedGetClient,
7879
getScope: () => {
7980
return {
81+
getPropagationContext: () => ({
82+
traceId: '1234567890abcdef1234567890abcdef',
83+
spanId: '1234567890abcdef',
84+
sampled: false,
85+
}),
8086
getSpan: () => {
8187
return {
8288
transaction: {
@@ -371,7 +377,7 @@ describe('wrapLoadWithSentry', () => {
371377
mockedBrowserTracing.options.traceFetch = true;
372378
});
373379

374-
it("doesn't create a span nor propagate headers, if `shouldCreateSpanForRequest` returns false", async () => {
380+
it("doesn't create a span if `shouldCreateSpanForRequest` returns false", async () => {
375381
mockedBrowserTracing.options.shouldCreateSpanForRequest = () => false;
376382

377383
const wrappedLoad = wrapLoadWithSentry(load);
@@ -391,10 +397,6 @@ describe('wrapLoadWithSentry', () => {
391397
expect.any(Function),
392398
);
393399

394-
expect(mockedSveltekitFetch).toHaveBeenCalledWith(
395-
...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}],
396-
);
397-
398400
mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true;
399401
});
400402

0 commit comments

Comments
 (0)
0