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

offline integration requires typescript dom lib in integration package #2877

Closed
jstewmon opened this issue Sep 2, 2020 · 13 comments
Closed

Comments

@jstewmon
Copy link

jstewmon commented Sep 2, 2020

Package + Version

  • [ x ] @sentry/integrations 5.22.3

Description

The recently added offline integration (#2778) depends on the TypeScript dom lib, which cannot be safely included in node.js projects due to incompatible interface definitions, such as Timers.

I found an existing mention of this problem, but no issue tracking it yet:

#2853 (comment)

I see that dom is included in the repo-wide tsconfig.json file:
https://github.com/getsentry/sentry-javascript/blob/master/packages/typescript/tsconfig.json#L9

It may help to remove dom from that shared file and only include dom in the browser package to avoid this problem.

@davidmyersdev
Copy link
Contributor

@jstewmon since I added the Offline integration, I would be happy to help out here if possible. Do you have an example of how to reproduce this problem?

@davidmyersdev
Copy link
Contributor

davidmyersdev commented Sep 9, 2020

Not directly related, but previous changes to this piece of the codebase lead to #2868, so we'll want to keep that in mind.

@jstewmon
Copy link
Author

jstewmon commented Sep 9, 2020

@voraciousdev sure, I put up a minimal reproduction repo here: https://github.com/jstewmon/sentry-javascript-2877

If you run npm i && npx tsc -b, you'll see the errors. The only way around it currently is to enable skipLibCheck to avoid checking the dom types referenced by localforage.

@jstewmon
Copy link
Author

jstewmon commented Nov 9, 2020

@HazAT @voraciousdev @kamilogorek The sentry packages have been essentially unusable in typescript projects targeting node for 2 months now. Is there any plan to remediate this issue?

@kirillgroshkov
Copy link

We've pinned to 5.21.4 (the last working version) about 2 months ago

@brandonb927
Copy link

brandonb927 commented Jan 12, 2021

Just chiming in here to say that we tried to upgrade to latest version this morning in our Typescript project and cannot do so because of the dom lib issue. Hoping for a timely resolution as there are a lot of fixes between 5.19.x (which we're on) and latest.

@kirillgroshkov I think we were able to get around this by adding dom to the tsconfig#compilerOptions in our project:

"compilerOptions": {
  "lib": [
    "dom"
  ]
}

I know this doesn't work for everyone, but it solved the errors we were seeing like:

node_modules/localforage/typings/localforage.d.ts(65,43): error TS2304: Cannot find name 'Blob'.
node_modules/localforage/typings/localforage.d.ts(67,54): error TS2304: Cannot find name 'Blob'.

After going through this, I don't actually think the issue we were encountering is the same as OP at this point, but I will leave this comment for anyone else that is curious.

@kirillgroshkov
Copy link

Yeah, we're still on 5.21.4 as a last working version

@kamilogorek
Copy link
Contributor

I just tested one of the possible solutions to this issue. Basically using Triple-Slash Directive /// <reference lib="dom" /> instead of relying on project config - https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html
The only downside here, is that it will include the built-in typing compilation-wide, not only in this specific file.

@dearsaturn
Copy link

dearsaturn commented Apr 26, 2021

Is there an acceptable fix for this that doesn't include adding dom typings? I'm using Sentry in a backend service, adding typings that aren't supported by what's actually available in Node doesn't seem like the right way.

@felipeochoa
Copy link

Bump from a paying customer

@AbhiPrasad
Copy link
Member

Hey folks, it's been a while, appreciate all the comments. We'll be addressing this as part of our work in #4240! Added this to the roadmap!

@eyal-confetti
Copy link

You can close this issue

@lobsterkatie
Copy link
Member

Hi, all. I believe this was fixed in #3508, which was included in v6.3.6. I'm going to close this, but please feel free to comment if you're still having trouble and we can reopen it if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants