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

Support different JS runtimes #5611

Open
AbhiPrasad opened this issue Aug 19, 2022 · 14 comments
Open

Support different JS runtimes #5611

AbhiPrasad opened this issue Aug 19, 2022 · 14 comments

Comments

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Aug 19, 2022

Problem Statement

The Sentry SDK should work with WinterCG-compatible runtimes other than NodeJS. We should build new SDKs for these platforms, or figure out how to adapt our Node SDK to fit them.

The spec: https://github.com/wintercg/proposal-common-minimum-api

There are also other JS runtimes we can take a look at:

Solution Brainstorm

If you are interested in this, please comment on the individual issues linked above!

@timfish

This comment was marked as outdated.

@timfish
Copy link
Collaborator

timfish commented Sep 16, 2022

The first step in supporting other runtimes is getting core, hub, types and utils compiling without dom/node types:

At this point, core, hub and types are compiling without DOM/node lib types.

However, utils still requires DOM and node lib types to build successfully.

To rectify this, all runtime specific code should be moved to the runtime packages:

  • - Move all browser specific code from @sentry/utils to @sentry/browser/utils
    • - Move supportsErrorEvent, supportsDOMError, supportsDOMException, supportsFetch, supportsNativeFetch, supportsReferrerPolicy and supportsHistory to @sentry/browser
    • - Move some functions (like getLocationHref and getDomElement) from browser.ts to @sentry/browser
    • - Move everything in instrument.ts to @sentry/browser
      • ❌ This is an issue due to addInstrumentationHandler usage in tracing
  • - Move all node specific code from @sentry/utils to @sentry/node
    • - Move everything in node.ts to @sentry/node
      • ❌ Moving dynamicRequire is an issue since it's used in tracing

The above can done without technically making breaking changes since we make no guarantees about @sentry/utils, but it will impact downstream SDKs that make use of some of the moved functions.

The major downside to moving these utilities to the browser/node packages and exporting them at the root is that they would become public API methods (for downstream SDKs to use) and with that comes stability guarantees.

It would be less of a maintenance headache to either:

  • Move them to new packages (like @sentry/browser-utils) or package export (@sentry/browser/utils)
  • Wrap them in an internal object export that we make no stability promises about

@sentry/tracing

There are a few issues above where @sentry/tracing relies on code from @sentry/utils which will get moved to browser/node. This will result in tracing depending on both browser/node. This can further be fixed by splitting the tracing code #5815.

AbhiPrasad pushed a commit that referenced this issue Sep 26, 2022
- Replaces `getGlobalObject` global object detection with a modern implementation taken from `core-js` which is then modified/simplified a little
- Prioritises `globalThis` which was previously removed due to requiring a TypeScript version which was required in a no longer supported version of Angular (#2978)
  - This is a requirement for future support of other js runtimes (#5611)
- No longer uses `isNodeEnv` 🎉
- Caches the global so it doesn't need to be evaluated for every invocation of `getGlobalObject`
@lforst
Copy link
Member

lforst commented Sep 26, 2022

How about having these as named exports? @sentry/tracing, @sentry/tracing/browser and @sentry/tracing/node?

Shouldn't it be @sentry/browser/tracing instead of @sentry/tracing/browser? So we can depend on @sentry/browser from within @sentry/browser/tracing. Maning the tracing submodule in @sentry/browser is just an extension of @sentry/browser. (I worded that kinda confusing but I think it gets the point across).

Also, I'd just have all tracing functionality in @sentry/node by default, so there wouldn't be any need for @sentry/node/tracing.

@timfish
Copy link
Collaborator

timfish commented Sep 26, 2022

Yep, I need to update the above ramblings.

There's a more recent issue for the tracing changes.

Also, I'd just have all tracing functionality in @sentry/node by default, so there wouldn't be any need for @sentry/node/tracing.

Would we want to add all the hub extensions by default too?

We wouldn't want to run _autoloadDatabaseIntegrations in the Electron SDK because it hits app startup time significantly!

@lforst
Copy link
Member

lforst commented Sep 26, 2022

Continued thread over in more recent issue here: #5815 (comment)

@AbhiPrasad AbhiPrasad changed the title Support different JS runtimes WinterCG - Support different JS runtimes Oct 5, 2022
@AbhiPrasad
Copy link
Member Author

We had a discussion internally about the global object changes - @timfish could we look into if we can just use globalThis?

@timfish
Copy link
Collaborator

timfish commented Oct 7, 2022

could we look into if we can just use globalThis?

Eventually, but we'll need to wait for Node.js v12 to be our MSV.

image

And the Sentry supported browsers suggests we need to support Android 4.4 and IE10/11 so these would need to be polyfilled.

image

That means we can probably remove the window and self lines from this:

const GLOBAL =
(typeof globalThis == 'object' && isGlobalObj(globalThis)) ||
// eslint-disable-next-line no-restricted-globals
(typeof window == 'object' && isGlobalObj(window)) ||
(typeof self == 'object' && isGlobalObj(self)) ||
(typeof global == 'object' && isGlobalObj(global)) ||
(function (this: any) {
return this;
})() ||
{};

@lobsterkatie
Copy link
Member

LOL

image

Looks like we haven't updated this page in quite a long time. (EDIT: Heh. Turns out July-me already discovered that: getsentry/sentry-docs#5351)

I can say for sure we don't need to worry about Android 4.4. I did a quick google, and the first version which can handle, for example, object spread is Android 8. For IE11, we've already told people that they have to use one of our ES5 bundles, to which we could add a polyfill if necessary. But the Node 10 business is definitely a bigger blocker. I like your idea of at least removing the browser-specific parts.

@AbhiPrasad
Copy link
Member Author

We merged in #5831, could we rebase the rest of the PRs @timfish?

@pbteja1998
Copy link

I’m using Remix with Cloudflare Workers. It would be great if there is support for this.

There is this package that everyone uses when using Cloudflare Workers, it might be a good starting point to start work on Cloudflare Workers runtime.
https://github.com/robertcepa/toucan-js

@Lms24 Lms24 changed the title WinterCG - Support different JS runtimes Support different JS runtimes Jan 9, 2023
@huw
Copy link

huw commented Feb 18, 2023

I realised I didn’t post cross-post here last time—I have a pretty hacky solution for Cloudflare Workers with Toucan.

@KrisBraun
Copy link

🤞 for Cloudflare Pages support, which should be nearly identical to Cloudflare Workers, just with different handlers.

@lforst
Copy link
Member

lforst commented May 8, 2023

@KrisBraun Gonna happen soon™ 👀

@huw
Copy link

huw commented Jul 31, 2023

I’ve updated my Cloudflare Workers Gist for Toucan v3.2.0 and Remix v2. It’s a lot more complicated & hacky now thanks to Remix v2, but still very possible. Please reply there if you have any questions :)

AbhiPrasad pushed a commit that referenced this issue Apr 2, 2024
Now we've dropped Nove v12 we can simplify to `globalThis`:

#5611 (comment)
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue Apr 19, 2024
Now we've dropped Nove v12 we can simplify to `globalThis`:

getsentry#5611 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

9 participants