Skip to content

feat: Deno SDK #9206

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

Merged
merged 23 commits into from
Oct 12, 2023
Merged

feat: Deno SDK #9206

merged 23 commits into from
Oct 12, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Oct 9, 2023

Closes #3009

This creates a Deno SDK which bundles all code and dependencies into ./build/index.js along with bundled types at ./build/index.d.ts.

There is a console prompt on startup if you haven't granted permissions for net access to the Sentry host:

Sentry SDK requires 'net' permission to send events.
Run with '--allow-net=o447951.ingest.sentry.io' to grant the requires permissions.

It might be worth adding another prompt if we can't read the source files otherwise ContextLines won't be able to load anything.

Other points to note:

  • The integrations mostly came from my previous deno work
  • We require @rollup/plugin-commonjs because lru_map is commonjs. Vendoring lru_map will simplify this
  • The new package is set as "private": true, since we don't want to publish this to npm

Tasks:

  • fetch instrumentation is not working
  • console instrumentation will not be working in modules loaded via npm:* specifiers
    • We can get the node global via import { GLOBAL_OBJ } from 'npm:@sentry/utils'
  • Script to Install Deno locally
  • Basic tests
  • deno.land publishing script
    • We can't use the default deploy web-hook because our library is compiled and not stored compiled in the repo
  • Readme

@timfish timfish marked this pull request as ready for review October 10, 2023 19:56
@timfish timfish requested a review from lforst October 10, 2023 20:03
Copy link
Contributor

@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.

This is a first pass. It is quite a bit of feedback but don't let that distract from how awesome this is. Great job man! <3

{
files: ['./test/*.ts'],
rules: {
'import/no-unresolved': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

When we get rid of all the npm: prefixes we can probably enable this rule again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Everything in /test/* is Deno typescript.
Some imports use denos URL imports which eslint doesn't understand

Copy link
Contributor

Choose a reason for hiding this comment

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

Checks out!

const error = new Error();

addGlobalEventProcessor((event: Event): Event | null => {
const appRoot = getAppRoot(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

h: Can we put this in the surrounding scope? That way we don't have to do the work over and over again.

Copy link
Collaborator Author

@timfish timfish Oct 11, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I am blind. Sorry!


import { defaultIntegrations, DenoClient, Hub, Scope } from '../build/index.js';
import { getNormalizedEvent } from './normalize.ts';
import { makeTestTransport } from './transport.ts';
Copy link
Contributor

Choose a reason for hiding this comment

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

m: We can configure the tsconfig to adjust the resolution of these imports to allow for .ts endings. Right now TS is screaming.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll have a look. I have the Deno vscode extension installed so don't see these errors!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

allowImportingTsExtensions is TypeScript 5+ and we're on 4.9.5.

For now I've added the Deno vscode extension to the recommended extensions and only enabled if for packages/deno/test.

I've also had to disable eslint for the ./scripts directory because the parser can't deal with top level await.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okok this also makes sense. I briefly discussed in the team whether we should bump the repo TS version to 5 but there are some considerations with regards to how we build types. Let's stick with the extension for now! Thanks for experimenting here.

@timfish
Copy link
Collaborator Author

timfish commented Oct 12, 2023

The main thing I've learnt is that at least for TypeScript < v5, it's impossible to create a tsconfig that gives zero errors for valid Deno TypeScript code...

@lforst lforst merged commit 5140e73 into getsentry:develop Oct 12, 2023
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.

Support for Deno server
2 participants