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 typing issue with withSentryConfig and NextConfig #5967

Merged
merged 5 commits into from Oct 18, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Oct 14, 2022

Closes #4560

This was super nasty to fix!

I managed to fix the reported null issue and a couple of other minor differences too, I finally get stuck on this horror:

Argument of type 'NextConfig' is not assignable to parameter of type 'NextConfigObjectWithSentry | NextConfigFunctionWithSentry | undefined'.
  Type 'NextConfig' is not assignable to type 'NextConfigObjectWithSentry'.
    Type 'NextConfig' is not assignable to type '{ webpack?: WebpackConfigFunction | null | undefined; target?: "server" | "experimental-serverless-trace" | undefined; distDir?: string | undefined; basePath?: string | undefined; publicRuntimeConfig?: { [key: string]: unknown; } | undefined; pageExtensions?: string[] | undefined; }'.
      Types of property 'webpack' are incompatible.
        Type 'NextJsWebpackConfig | null | undefined' is not assignable to type 'WebpackConfigFunction | null | undefined'.
          Type 'NextJsWebpackConfig' is not assignable to type 'WebpackConfigFunction'.
            Types of parameters 'context' and 'options' are incompatible.
              Type 'BuildContext' is not assignable to type 'WebpackConfigContext'.
                Types of property 'config' are incompatible.
                  Type 'NextConfigObject' is not assignable to type 'NextConfigComplete'.
                    Type 'NextConfigObject' is missing the following properties from type 'Required<NextConfig>': exportPathMap, i18n, eslint, typescript, and 33 more.  ts(2345)

The issue is that NextJsWebpackConfig has the property config: NextConfigComplete and NextConfigComplete is Required<NextConfig>. I could go through and add the 33 missing properties to the vendored NextConfigObject but it will get broken as soon as new properties get added.

I spent an age trying to work out how to fix this and it basically comes down to the fact that Required is evil and makes creating vendored types close to impossible, especially when you rely on the detail of those vendored types in your own code.

I resigned myself to making the smallest change somewhere to any and found that I could do that with the config in BuildContext. So we don't lose type safety in the Sentry code, I had to add config as NextConfigObject where appropriate.

This PR also adds a test:types script which runs after the integration tests and ensures that this doesn't get broken in the future. It has a package.json that means that we can test against the next v12 types. I did try updating @sentry/nextjs to devDepend on v12 but it threw up a few errors and I wasn't convinced this was a good idea.

@timfish

This comment was marked as outdated.

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

Thank you Tim for fixing this! This looks good to me.

.gitignore Outdated Show resolved Hide resolved
@lforst
Copy link
Member

lforst commented Oct 18, 2022

E2E tests are failing because they're requiring secrets we don't have in forks. I opened #5986 to fix this.

@lforst lforst merged commit 8b2b78e into getsentry:master Oct 18, 2022
Lms24 added a commit that referenced this pull request Oct 18, 2022
@timfish timfish deleted the fix/#4560 branch November 29, 2022 09:37
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.

Typescript Mismatch
2 participants