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

feat(deno): Stop inlining types from core #14729

Merged
merged 8 commits into from
Dec 16, 2024
Merged

feat(deno): Stop inlining types from core #14729

merged 8 commits into from
Dec 16, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Dec 16, 2024

Extracted out from #14721

Today, we inline all the types into deno/build/index.d.ts. This used to work "OK" previously, where all the types came from @sentry/types and where thus all "simple types".

If we want to replace some of this with actual classes, e.g. Scope or Client classes, this starts to fail, because now we have separate definitions that do not match.

So to fix this, we now stop inlining all the types, but instead re-export them from @sentry/core. While at this, I also updated the test setup to ignore utils/types and just use core for everything.

With this change, deno/build/index.d.ts looks something like this:

import * as _sentry_core from '@sentry/core';
import { BaseTransportOptions, Options, TracePropagationTargets, ClientOptions, ServerRuntimeClient, Integration, Client } from '@sentry/core';
export { ... } from '@sentry/core';

// additional types from deno go here...

I do not think this should be breaking, but 🤷 hard to say for sure with these things.

@mydea mydea self-assigned this Dec 16, 2024
@mydea mydea changed the title feat(deno): Stop inlining types feat(deno): Stop inlining types from core Dec 16, 2024
@lforst
Copy link
Member

lforst commented Dec 16, 2024

I cannot remember whether we can externalize core in the declarations or not. From my pov it seems fine. @timfish do you remember?

@timfish
Copy link
Collaborator

timfish commented Dec 16, 2024

I'll be back at my desk in about an hour and I'll take a proper look

@timfish
Copy link
Collaborator

timfish commented Dec 16, 2024

I think we bundled the code (and types) for Deno because we were publishing to (the now deprecated) deno.land registry.

Deno 2 has almost perfect node/npm support so for v9 we could drop Deno v1 support and just publish to npm without any bundling of code or types?

@mydea
Copy link
Member Author

mydea commented Dec 16, 2024

I think we bundled the code (and types) for Deno because we were publishing to (the now deprecated) deno.land registry.

Deno 2 has almost perfect node/npm support so for v9 we could drop Deno v1 support and just publish to npm without any bundling of code or types?

This sounds reasonable to me! Making this easier & more streamlined 👍

@mydea mydea merged commit 88f486c into develop Dec 16, 2024
158 checks passed
@mydea mydea deleted the fn/deno-types branch December 16, 2024 15:19
mydea added a commit that referenced this pull request Dec 16, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
I noticed that after
#14729, now tests
started failing 😬

The reason:

* We run `deno check ./build/index.mjs`
* This checks the types, but we now do not inline those anymore into the
generated `./build/index.d.ts` file
* Instead, we just import them from `@sentry/core`
* Now `deno check` downloads the latest version of core into a local tmp
folder and uses this for checking
* But there are different exports there than locally, leading to test
failures 😬

this PR simple kills the type tests - not sure if we need them/they are
needed, but I can't think of a different way to make this work, there
isn't really a way (as far as I see?) to tell `deno check` to consider a
dependency from a local path instead 😬
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

4 participants