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

fix(remix): Support merging json responses from root loader functions. #5548

Merged
merged 7 commits into from
Aug 9, 2022

Conversation

onurtemizkan
Copy link
Collaborator

Resolves #5539

Introduces support of json responses while injecting sentryTrace and sentryBaggage into the root loader.
Added a set of tests to ensure that we don't omit user-defined loader responses.

@onurtemizkan onurtemizkan added the Package: remix Issues related to the Sentry Remix SDK label Aug 9, 2022
@onurtemizkan onurtemizkan added this to the Sentry Remix SDK milestone Aug 9, 2022
@onurtemizkan onurtemizkan self-assigned this Aug 9, 2022
@onurtemizkan onurtemizkan force-pushed the onur/remix-root-wrapper-responses branch from f3c0155 to 80d1711 Compare August 9, 2022 15:16
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Nice!

});
}

test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test('should inject `sentry-trace` and `baggage` into root loader returning `{}`.', async ({ page }) => {
test('should inject `sentry-trace` and `baggage` into root loader returning an empty object.', async ({ page }) => {

return redirect('/plain');
case 'return-redirect-json':
return redirect('/json');
}
Copy link
Member

Choose a reason for hiding this comment

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

do we need a default case here? Maybe log a warning if theres a type not defined here?


export function getRouteData(page: Page): Promise<any> {
return page.evaluate('window.__remixContext.routeData').catch(err => {
return {};
Copy link
Member

Choose a reason for hiding this comment

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

log out the error?

const envelope = await getEnvelopeRequest(url);
const transaction = envelope[2];

assertSentryTransaction(transaction, {
Copy link
Member

Choose a reason for hiding this comment

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

Why did we delete this test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We no longer have a case where there's no loader in root.tsx defined.
Might need to utilize multiple projects to be able to test those kind of conflicted scenarios. We will probably need a similar thing to test manual wrappers too.

Trying to figure out now, will add that to this PR, if it doesn't require a large structural change.

@onurtemizkan
Copy link
Collaborator Author

@AbhiPrasad I'm converting this to draft for now. :( Apart from the required test structure updates, it also seems the redirect cases are not running correctly.

@onurtemizkan onurtemizkan marked this pull request as draft August 9, 2022 17:16
@AbhiPrasad
Copy link
Member

Can we still get the fix merged in to unblock users? We can always figure out the tests afterwards.

@onurtemizkan
Copy link
Collaborator Author

Can we still get the fix merged in to unblock users?

Sure, also fixed the problem about redirect responses.
Ready for review again 👍

@onurtemizkan onurtemizkan marked this pull request as ready for review August 9, 2022 17:53
const data = await extractData(res);

if (typeof data === 'object') {
return { ...data, ...traceAndBaggage };
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to even return traceAndBaggage if it's a response? Couldn't we just just return the res?

Copy link
Member

Choose a reason for hiding this comment

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

Wait I'm dumb - yes we do, nvm.

if (isResponse(res) && !isRedirectResponse(res) && !isCatchResponse(res)) {
const data = await extractData(res);

if (typeof data === 'object') {
Copy link
Collaborator Author

@onurtemizkan onurtemizkan Aug 9, 2022

Choose a reason for hiding this comment

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

@AbhiPrasad, the else case of this is a bit weird (when data is primitive). It doesn't sound right to return a primitive from a loader, but TS definitions allow it.

So, would it make sense if we skip injection in such cases?
It may mess up the response when we spread it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would rather we skip injection here.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Let's try to figure out the loader not being defined case later (for the deleted test) - I'm comfortable shipping what we got for now!

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) August 9, 2022 18:30
@AbhiPrasad AbhiPrasad merged commit 9d38065 into master Aug 9, 2022
@AbhiPrasad AbhiPrasad deleted the onur/remix-root-wrapper-responses branch August 9, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: remix Issues related to the Sentry Remix SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

makeWrappedRootLoader does not handle Response cases
2 participants