Skip to content

Conversation

krystofwoldrich
Copy link
Contributor

@krystofwoldrich krystofwoldrich commented Sep 6, 2023

The downleveled types were not shipped.
The mapping of types to the downleveled types had an incorrect path with the buildDir prefix.

Before this change the sentry-react-native typescript 3.8 test failed on :

 ❯ yarn type-check                
yarn run v1.22.19
$ tsc --project tsconfig.build.json
../../../node_modules/@sentry/types/types/span.d.ts:8:43 - error TS1110: Type expected.

8 export type SpanOrigin = SpanOriginType | `${SpanOriginType}.${SpanOriginCategory}` | `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}` | `${SpanOriginType}.${SpanOriginCategory}.${SpanOriginIntegrationName}.${SpanOriginIntegrationPart}`;
                                            ~~~


Found 1 error.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Todo

  • After these changes the types are shipped and used but something is off as with the default types I have no type errors in sentry-react-native but with the 3.8 types I have 58 type errors. This was caused by .yalc not linking transitive deps (@sentry/browser -> @sentry/core)
  • Add/fix compile tests with TS 3.8 to ensure downleveled types are picked up by TS and are compatible

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

I think the .npmignore in packages/gatsby also needs adjusting. Would you mind taking a look at it? Should be the same change as in the root one.

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

damn, good catch!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks! RE tests, great idea but if we wanna ship this quickly, I suggest we take care of them in a follow-up PR.

@krystofwoldrich
Copy link
Contributor Author

@Lms24 @mydea I've fixed the missing gatsby npmignore and I'll add the test in a next PR.

@krystofwoldrich krystofwoldrich marked this pull request as ready for review September 6, 2023 09:07
@krystofwoldrich
Copy link
Contributor Author

Here is the follow-up PR
#8955

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 75.35 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.24 KB (+0.03% 🔺)
@sentry/browser - Webpack (gzipped) 21.84 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 69.87 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.19 KB (+0.06% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.18 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.81 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 85.09 KB (+0.08% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 59.73 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.08 KB (+0.03% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 75.37 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped) 21.88 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 93.23 KB (+0.01% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 50.81 KB (+0.03% 🔺)

@krystofwoldrich krystofwoldrich merged commit 8766bdd into develop Sep 6, 2023
@krystofwoldrich krystofwoldrich deleted the kw-fix-downleveled-types branch September 6, 2023 10:09
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.

3 participants