Sentry example: add js types to SentryWebpackPluginOptions #28726

jonespen · 2021-09-02T05:26:36Z

Enables code completion in Visual Studio Code (and perhaps other editors)

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes

Enables code completion in Visual Studio Code (and perhaps other editors)

huozhi

Thanks!

belgattitude · 2021-09-02T23:02:56Z

Looks great,

But I don't think it works if you enable // @ts-check in your next.config.js

Could you try ?

The type SentryWebpackPluginOptions is a re-export from SentryCliPluginOptions from @sentry/webpack-plugin

When the options are typed, it will require the "include" property

See https://github.com/getsentry/sentry-webpack-plugin/blob/ed4be5a166e8066019dc6e1cf198106126721e86/index.d.ts#L30

Not sure if it can be changed upstream ...

Any idea ?

jonespen · 2021-09-03T05:59:29Z

@belgattitude thanks for the heads up! I have never used // @ts-check before in next.config.js, but looks handy. Makes my whole config red tho, but thats my problem ;)

Anyway, you're totally right, in @sentry/nextjs they use Partial<SentryWebpackPluginOptions> as the options type. By changing the jsdoc string to /** @type {Partial<import('@sentry/nextjs/dist/config/types').SentryWebpackPluginOptions>} */, it seems to match the expected type. I'll update this PR.

This matches the expected type in https://github.com/getsentry/sentry-javascript/blob/e71454ef781563e36c727315d1a5b59acb9e2e0a/packages/nextjs/src/config/webpack.ts#L38

belgattitude · 2021-09-03T08:21:39Z

Work perfect. That's great I didn't know we could add Partial there.

So all fine...

BTW, I would studlyCaps the const: const sentryWebpackPluginOptions.

lobsterkatie · 2021-09-08T01:21:45Z

BTW, I would studlyCaps the const: const sentryWebpackPluginOptions.

@belgattitude, I am making some other small changes to the example, and have included that fix: #28359

ijjk · 2022-02-09T20:04:48Z

Failing test suites

Commit: 8bee034

yarn testheadless test/integration/custom-routes/test/index.test.js

Custom routes > dev mode > should handle external beforeFiles rewrite correctly
Custom routes > dev mode > should match has query redirect with duplicate query key
Custom routes > server mode > should match has query redirect with duplicate query key
Custom routes > serverless mode > should match has query redirect with duplicate query key

Expand output

● Custom routes › dev mode › should handle external beforeFiles rewrite correctly

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  82 |   })
  83 |
> 84 |   it('should handle external beforeFiles rewrite correctly', async () => {
     |   ^
  85 |     const res = await fetchViaHTTP(appPort, '/overridden')
  86 |     const html = await res.text()
  87 |

  at runTests (integration/custom-routes/test/index.test.js:84:3)
  at integration/custom-routes/test/index.test.js:2166:5
  at integration/custom-routes/test/index.test.js:2138:3
  at Object.<anonymous> (integration/custom-routes/test/index.test.js:2109:1)

● Custom routes › dev mode › should match has query redirect with duplicate query key

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
    "hello": Array [
      "world",
      "another",
    ],
-   "value": "another",
+   "value": "world",
  }

  1107 |
  1108 |     expect(parsed.pathname).toBe('/somewhere')
> 1109 |     expect(parsed.query).toEqual({
       |                          ^
  1110 |       hello: ['world', 'another'],
  1111 |       value: 'another',
  1112 |     })

  at Object.<anonymous> (integration/custom-routes/test/index.test.js:1109:26)
      at runMicrotasks (<anonymous>)

● Custom routes › server mode › should match has query redirect with duplicate query key

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
    "hello": Array [
      "world",
      "another",
    ],
-   "value": "another",
+   "value": "world",
  }

  1107 |
  1108 |     expect(parsed.pathname).toBe('/somewhere')
> 1109 |     expect(parsed.query).toEqual({
       |                          ^
  1110 |       hello: ['world', 'another'],
  1111 |       value: 'another',
  1112 |     })

  at Object.<anonymous> (integration/custom-routes/test/index.test.js:1109:26)
      at runMicrotasks (<anonymous>)

● Custom routes › serverless mode › should match has query redirect with duplicate query key

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
    "hello": Array [
      "world",
      "another",
    ],
-   "value": "another",
+   "value": "world",
  }

  1107 |
  1108 |     expect(parsed.pathname).toBe('/somewhere')
> 1109 |     expect(parsed.query).toEqual({
       |                          ^
  1110 |       hello: ['world', 'another'],
  1111 |       value: 'another',
  1112 |     })

  at Object.<anonymous> (integration/custom-routes/test/index.test.js:1109:26)
      at runMicrotasks (<anonymous>)

Read more about building and testing Next.js in contributing.md.

kachkaev · 2022-02-09T20:14:06Z

Using this in https://github.com/dtpstat/website/blob/c3c4b07ccaf3e8c400ef2c9523d1517d0277fe8c/next.config.mjs#L80-L83. Works like a charm, so thanks for updating the example!

ijjk · 2022-02-10T02:39:57Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
buildDuration	16.7s	19s	⚠️ +2.3s
buildDurationCached	6.3s	6.8s	⚠️ +512ms
nodeModulesSize	359 MB	359 MB	⚠️ +13 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.748	3.999	⚠️ +0.25
/ avg req/sec	666.97	625.1	⚠️ -41.87
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.772	1.989	⚠️ +0.22
/error-in-render avg req/sec	1410.48	1257.19	⚠️ -153.29

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	27.9 kB	27.9 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.5 kB	71.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.57 kB	2.57 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	5.01 kB	5.01 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.26 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
buildDuration	21.9s	22.2s	⚠️ +265ms
buildDurationCached	7.3s	7s	-331ms
nodeModulesSize	359 MB	359 MB	⚠️ +13 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.042	3.974	-0.07
/ avg req/sec	618.45	629.03	+10.58
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.033	2.063	⚠️ +0.03
/error-in-render avg req/sec	1229.52	1211.67	⚠️ -17.85

Client Bundles (main, webpack, commons)

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.1 kB	42.1 kB	✓
main-HASH.js gzip	27.9 kB	27.9 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.6 kB	71.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.56 kB	2.56 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	5.05 kB	5.05 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.28 kB	2.28 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	Skn0tt/next.js patch-1	Change
index.html gzip	533 B	533 B	✓
link.html gzip	547 B	547 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: ab71b53

jonespen · 2022-02-10T08:07:05Z

@lobsterkatie wdyt, is it safe to import types from @sentry/nextjs like this?

lobsterkatie · 2022-02-14T20:18:17Z

@lobsterkatie wdyt, is it safe to import types from @sentry/nextjs like this?

As far as I know, but I'll admit, I don't have much experience with that syntax. ( It seems like the folks above have it working, though.)

But I'm a little confused why this is even necessary. I haven't done anything special that I know of to my test app, and I get types on the config object:

jonespen · 2022-02-15T07:22:15Z

@lobsterkatie ah, it works if you define the Sentry options inline, and not create a sentryWebpackPluginOptions const upfront. Perhaps we should just update the with-sentry example to define the options inline?

lobsterkatie · 2022-02-16T19:53:53Z

@lobsterkatie ah, it works if you define the Sentry options inline, and not create a sentryWebpackPluginOptions const upfront. Perhaps we should just update the with-sentry example to define the options inline?

Oh, okay. Yeah, this makes sense actually. When it's part of the withSentry function, it's an argument being passed to a function which has full typing. When it's just a random variable there's no way for VSCode (etc) to know what you intend to do with it, unless you do the docstring annotation as above (or, of course, write your file in TS, and give it a type when you create it).

As to the correct fix, one easy thing I can do is to export that type. Past that, I don't have a strong opinion on which way it should be written. If you are going to go the docstring route, though, I'd wait until the type is exported, because it will make it much cleaner. I'm going to push a PR for that now, and it'll go out in the next release (which will either be 6.17.10 or 6.18).

This will allow people who want typing in their `next.config.js` file to have it without digging into `dist`. See vercel/next.js#28726.

jonespen · 2022-02-17T08:01:08Z

Perfect, thanks! I'll update this PR when it is released.

balazsorban44 · 2022-02-19T03:32:51Z

My suggestion here would be to just inline the config, then no extra import is needed 👍

jonespen · 2022-02-21T10:14:40Z

@balazsorban44 good call, I guess users who need to extract the config can get the TS type from @sentry/nextjs if needed.

(Also, nice sweater!)

balazsorban44 · 2022-02-21T20:54:35Z

Thanks for the PR and thanks about the sweater! 🇳🇴

Add js types to SentryWebpackPluginOptions

16f80fe

Enables code completion in Visual Studio Code (and perhaps other editors)

jonespen requested review from ijjk, leerob, lfades, shuding and timneutkens as code owners September 2, 2021 05:26

ijjk added the examples Issue/PR related to examples label Sep 2, 2021

huozhi previously approved these changes Sep 2, 2021

View reviewed changes

Use partial type for SentryWebpackPluginOptions

17c85fa

This matches the expected type in https://github.com/getsentry/sentry-javascript/blob/e71454ef781563e36c727315d1a5b59acb9e2e0a/packages/nextjs/src/config/webpack.ts#L38

jonespen dismissed huozhi’s stale review via 17c85fa September 3, 2021 06:01

jonespen requested a review from huozhi September 3, 2021 06:12

huozhi previously approved these changes Sep 3, 2021

View reviewed changes

belgattitude mentioned this pull request Sep 3, 2021

chore(next.config): typing sentry options belgattitude/nextjs-monorepo-example#367

Merged

Merge branch 'canary' into patch-1

401a16a

jonespen dismissed huozhi’s stale review via 401a16a February 10, 2022 08:05

jonespen requested a review from steven-tey as a code owner February 10, 2022 08:05

lobsterkatie mentioned this pull request Feb 16, 2022

fix(nextjs): Export SentryWebpackPluginOptions type getsentry/sentry-javascript#4597

Merged

jonespen added 2 commits February 21, 2022 11:09

Merge branch 'canary' into patch-1

514b827

inline sentry config

0ea8b79

balazsorban44 approved these changes Feb 21, 2022

View reviewed changes

balazsorban44 merged commit 31d8385 into vercel:canary Feb 21, 2022

jonespen deleted the patch-1 branch February 23, 2022 21:10

github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sentry example: add js types to SentryWebpackPluginOptions #28726

Sentry example: add js types to SentryWebpackPluginOptions #28726

jonespen commented Sep 2, 2021 •

edited

Loading

huozhi left a comment

belgattitude commented Sep 2, 2021 •

edited

Loading

jonespen commented Sep 3, 2021

belgattitude commented Sep 3, 2021

lobsterkatie commented Sep 8, 2021

ijjk commented Feb 9, 2022

kachkaev commented Feb 9, 2022

ijjk commented Feb 10, 2022

jonespen commented Feb 10, 2022

lobsterkatie commented Feb 14, 2022

jonespen commented Feb 15, 2022

lobsterkatie commented Feb 16, 2022

jonespen commented Feb 17, 2022

balazsorban44 commented Feb 19, 2022

jonespen commented Feb 21, 2022

balazsorban44 commented Feb 21, 2022

Sentry example: add js types to SentryWebpackPluginOptions #28726

Sentry example: add js types to SentryWebpackPluginOptions #28726

Conversation

jonespen commented Sep 2, 2021 • edited Loading

Bug

Feature

Documentation / Examples

huozhi left a comment

Choose a reason for hiding this comment

belgattitude commented Sep 2, 2021 • edited Loading

jonespen commented Sep 3, 2021

belgattitude commented Sep 3, 2021

lobsterkatie commented Sep 8, 2021

ijjk commented Feb 9, 2022

Failing test suites

kachkaev commented Feb 9, 2022

ijjk commented Feb 10, 2022

Stats from current PR

jonespen commented Feb 10, 2022

lobsterkatie commented Feb 14, 2022

jonespen commented Feb 15, 2022

lobsterkatie commented Feb 16, 2022

jonespen commented Feb 17, 2022

balazsorban44 commented Feb 19, 2022

jonespen commented Feb 21, 2022

balazsorban44 commented Feb 21, 2022

jonespen commented Sep 2, 2021 •

edited

Loading

belgattitude commented Sep 2, 2021 •

edited

Loading