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(nextjs): Fix faulty import in Next.js .d.ts #7175

Merged
merged 4 commits into from Feb 14, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Feb 14, 2023

This came up in discord: https://discord.com/channels/621778831602221064/621786575591702529/1074841610753413261

Screenshot 2023-02-14 at 13 41 34

Our Next.js types currently have references to the Node package but the paths in the reference contain our internal build structure which leads to mismatches when you use the package outside of our monorepo.

https://unpkg.com/browse/@sentry/nextjs@7.37.2/types/index.types.d.ts

I didn't know how to fix this properly via settings and stuff but slightly changing how we import things seemed to get rid of these import() statements.

Also added an import that would have failed to our E2E tests.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

Replay SDK metrics 🚀

    Plain +Sentry +Replay
Revision Value Value Diff Ratio Value Diff Ratio
LCP This PR a993786 118.42 ms 125.10 ms +6.68 ms +5.64 % 176.16 ms +57.74 ms +48.76 %
Previous e9eec27 67.47 ms 102.63 ms +35.17 ms +52.13 % 128.85 ms +61.38 ms +90.98 %
CLS This PR a993786 0.06 ms 0.06 ms -0.00 ms -0.03 % 0.06 ms -0.00 ms -0.33 %
Previous e9eec27 0.06 ms 0.06 ms +0.00 ms +0.07 % 0.06 ms -0.00 ms -0.33 %
CPU This PR a993786 27.13 % 28.59 % +1.46 pp +5.37 % 36.67 % +9.54 pp +35.15 %
Previous e9eec27 19.40 % 19.02 % -0.37 pp -1.92 % 25.44 % +6.04 pp +31.16 %
JS heap avg This PR a993786 1.93 MB 1.99 MB +63.78 kB +3.30 % 2.86 MB +931.83 kB +48.28 %
Previous e9eec27 1.94 MB 1.99 MB +45.82 kB +2.36 % 2.87 MB +927.84 kB +47.75 %
JS heap max This PR a993786 2.32 MB 2.59 MB +266.27 kB +11.48 % 3.36 MB +1.04 MB +44.94 %
Previous e9eec27 2.3 MB 2.55 MB +249.12 kB +10.82 % 3.35 MB +1.05 MB +45.46 %
netTx This PR a993786 0 B 0 B 0 B n/a 2.21 kB +2.21 kB n/a
Previous e9eec27 0 B 0 B 0 B n/a 2.21 kB +2.21 kB n/a
netRx This PR a993786 0 B 0 B 0 B n/a 41 B +41 B n/a
Previous e9eec27 0 B 0 B 0 B n/a 41 B +41 B n/a
netCount This PR a993786 0 0 0 n/a 1 +1 n/a
Previous e9eec27 0 0 0 n/a 1 +1 n/a
netTime This PR a993786 0.00 ms 0.00 ms 0.00 ms n/a 109.41 ms +109.41 ms n/a
Previous e9eec27 0.00 ms 0.00 ms 0.00 ms n/a 88.58 ms +88.58 ms n/a

Previous results on branch: develop

RevisionLCPCLSCPUJS heap avgJS heap maxnetTxnetRxnetCountnetTime
e9eec27+61.38 ms-0.00 ms+6.04 pp+927.84 kB+1.05 MB+2.21 kB+41 B+1+88.58 ms
d604022+58.83 ms-0.00 ms+7.65 pp+930.16 kB+1.05 MB+2.21 kB+41 B+1+109.63 ms
a961e57+54.75 ms-0.00 ms+6.50 pp+929.18 kB+1.07 MB+2.21 kB+41 B+1+92.73 ms
f7c0a2f+46.14 ms+0.00 ms+6.37 pp+921.47 kB+1.06 MB+2.23 kB+41 B+1+207.30 ms
cb19818+57.16 ms+0.00 ms+11.95 pp+1.07 MB+2.21 MB+2.52 kB+41 B+1+111.50 ms
ee301c3+71.07 ms-0.00 ms+12.64 pp+1.07 MB+2.22 MB+2.55 kB+41 B+1+94.67 ms
93c4759+61.10 ms-0.00 ms+12.72 pp+1.08 MB+2.19 MB+2.57 kB+41 B+1+116.75 ms
274f489+63.60 ms-0.00 ms+11.56 pp+1.08 MB+2.2 MB+2.56 kB+41 B+1+116.60 ms
4827b60+58.67 ms+0.00 ms+18.38 pp+1.07 MB+2.22 MB+2.6 kB+41 B+1+91.21 ms
c3806eb+79.85 ms-0.00 ms+12.10 pp+1.05 MB+2.16 MB+2.54 kB+41 B+1+93.58 ms

*) pp - percentage points - an absolute difference between two percentages.
Last updated: Tue, 14 Feb 2023 13:16:46 GMT

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.09 KB (+0.06% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.23 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.71 KB (+0.06% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55.37 KB (+0.04% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.44 KB (+0.03% 🔺)
@sentry/browser - Webpack (minified) 66.81 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.47 KB (+0.03% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.9 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.03 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.29 KB (+0.06% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.51 KB (+0.06% 🔺)
@sentry/replay - Webpack (gzipped + minified) 36.67 KB (-0.6% 🔽)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.23 KB (+0.04% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 53.77 KB (+0.05% 🔺)

@lforst
Copy link
Member Author

lforst commented Feb 14, 2023

This is all just a big learning process. Will come in handy when tackling more full-stack frameworks.

@lforst lforst marked this pull request as ready for review February 14, 2023 12:51
@@ -15,6 +15,8 @@ import { addOrUpdateIntegration } from '../common/userIntegrations';
import { isBuild } from './utils/isBuild';

export * from '@sentry/node';
export { Integrations };
Copy link
Member

Choose a reason for hiding this comment

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

why do we have to explicitly export, doesn't the wildcard above take care of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah wait I think this is a leftover from messing around... Lemme check if I can remove it or if it breaks again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, wasn't needed --> I removed it.

lforst and others added 2 commits February 14, 2023 13:03
…rver.config.ts

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
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.

Makes sense. I wonder if we should lint for this 🤔

@lforst
Copy link
Member Author

lforst commented Feb 14, 2023

Makes sense. I wonder if we should lint for this 🤔

Linting is already pretty strong here - ts does a somewhat good job. The weird thing was just the build output...

One thing I had in mind once was having snapshot tests for our type definitions. Every time they change, we should have to sign off in order to ensure no unintended breakage.

@lforst lforst merged commit 1921888 into develop Feb 14, 2023
@lforst lforst deleted the lforst-fix-nextjs-integrations-types branch February 14, 2023 13:25
ramchaik pushed a commit to ramchaik/sentry-javascript that referenced this pull request Feb 14, 2023
Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
@AbhiPrasad
Copy link
Member

I like the idea of snapshot tests! Let's open a ticket?

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.

None yet

2 participants