rehype-mathjax should not have a dependency on @types/web

Question

rehype-mathjax should not have a dependency on @types/web

FunkMonkey opened this issue 2 years ago · 13 comments

FunkMonkey commented 2 years ago

Initial checklist

I read the support docs
I read the contributing guide
I agree to follow the code of conduct
I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

4.0.2

Link to runnable example

No response

Steps to reproduce

Install reyhpe-mathjax via a package manager

Expected behavior

The typescript project referencing rehype-mathjax should not throw any new errors.

Actual behavior

Typescript (e.g. in VSCode) gives the following errors for@types/web and @typescript/lib-dom, because we basically have double definitions.

Should rehype-mathjax not only have @types/web as a devDependency or peerDependency so it does not interfere with the typescript setup of the project that uses it?

Why is the specific version of @types/web here required anyway?

Definitions of the following identifiers conflict with those in another file: NodeFilter, XPathNSResolver, SVGMatrix, WebKitCSSMatrix, SVGPoint, SVGRect, location, webkitURL, ImportExportKind, TableKind, ValueType, ExportValue, Exports, ImportValue, Imports, ModuleImports, ElementTagNameMap, name, AlgorithmIdentifier, BigInteger, BinaryData, BlobPart, BodyInit, BufferSource, COSEAlgorithmIdentifier, CSSNumberish, CanvasImageSource, ClipboardItemData, ClipboardItems, ConstrainBoolean, ConstrainDOMString, ConstrainDouble, ConstrainULong, DOMHighResTimeStamp, EpochTimeStamp, EventListenerOrEventListenerObject, Float32List, FormDataEntryValue, GLbitfield, GLboolean, GLclampf, GLenum, GLfloat, GLint, GLint64, GLintptr, GLsizei, GLsizeiptr, GLuint, GLuint64, HTMLOrSVGImageElement, HTMLOrSVGScriptElement, HashAlgorithmIdentifier, HeadersInit, IDBValidKey, ImageBitmapSource, InsertPosition, Int32List, LineAndPositionSetting, MediaProvider, MessageEventSource, MutationRecordType, NamedCurve, OnBeforeUnloadEventHandler, OnErrorEventHandler, PerformanceEntryList, ReadableStreamController, ReadableStreamReader, RenderingContext, RequestInfo, TexImageSource, TimerHandler, Transferable, Uint32List, UvmEntries, UvmEntry, VibratePattern, WindowProxy, XMLHttpRequestBodyInit, AlignSetting, AnimationPlayState, AnimationReplaceState, AppendMode, AttestationConveyancePreference, AudioContextLatencyCategory, AudioContextState, AuthenticatorAttachment, AuthenticatorTransport, AutoKeyword, AutomationRate, BinaryType, BiquadFilterType, CanPlayTypeResult, CanvasDirection, CanvasFillRule, CanvasFontKerning, CanvasFontStretch, CanvasFontVariantCaps, CanvasLineCap, CanvasLineJoin, CanvasTextAlign, CanvasTextBaseline, CanvasTextRendering, ChannelCountMode, ChannelInterpretation, ClientTypes, ColorGamut, ColorSpaceConversion, CompositeOperation, CompositeOperationOrAuto, CredentialMediationRequirement, DOMParserSupportedType, DirectionSetting, DisplayCaptureSurfaceType, DistanceModelType, DocumentReadyState, EndOfStreamError, EndingType, FillMode, FontFaceLoadStatus, FontFaceSetLoadStatus, FullscreenNavigationUI, GamepadHapticActuatorType, GamepadMappingType, HdrMetadataType, IDBCursorDirection, IDBRequestReadyState, IDBTransactionDurability, IDBTransactionMode, ImageOrientation, ImageSmoothingQuality, IterationCompositeOperation, KeyFormat, KeyType, KeyUsage, LineAlignSetting, MediaDecodingType, MediaDeviceKind, MediaEncodingType, MediaKeyMessageType, MediaKeySessionClosedReason, MediaKeySessionType, MediaKeyStatus, MediaKeysRequirement, MediaSessionAction, MediaSessionPlaybackState, MediaStreamTrackState, NotificationDirection, NotificationPermission, OrientationLockType, OrientationType, OscillatorType, OverSampleType, PanningModelType, PaymentComplete, PermissionName, PermissionState, PlaybackDirection, PositionAlignSetting, PredefinedColorSpace, PremultiplyAlpha, PresentationStyle, PublicKeyCredentialType, PushEncryptionKeyName, RTCBundlePolicy, RTCDataChannelState, RTCDegradationPreference, RTCDtlsTransportState, RTCIceCandidateType, RTCIceComponent, RTCIceConnectionState, RTCIceCredentialType, RTCIceGathererState, RTCIceGatheringState, RTCIceProtocol, RTCIceTcpCandidateType, RTCIceTransportPolicy, RTCIceTransportState, RTCPeerConnectionState, RTCPriorityType, RTCRtcpMuxPolicy, RTCRtpTransceiverDirection, RTCSdpType, RTCSignalingState, RTCStatsIceCandidatePairState, RTCStatsType, ReadyState, RecordingState, ReferrerPolicy, RemotePlaybackState, RequestCache, RequestCredentials, RequestDestination, RequestMode, RequestRedirect, ResidentKeyRequirement, ResizeObserverBoxOptions, ResizeQuality, ResponseType, ScrollBehavior, ScrollLogicalPosition, ScrollRestoration, ScrollSetting, SecurityPolicyViolationEventDisposition, SelectionMode, ServiceWorkerState, ServiceWorkerUpdateViaCache, ShadowRootMode, SlotAssignmentMode, SpeechSynthesisErrorCode, TextTrackKind, TextTrackMode, TouchType, TransferFunction, UserVerificationRequirement, VideoFacingModeEnum, WebGLPowerPreference, WorkerType, XMLHttpRequestResponseType

Thanks a lot for your support!

Runtime

Node v16

Package manager

yarn 1

OS

Windows

Build and bundle tools

Vite

Answer 1 · 2022-07-08T16:28:13.000Z

dev-dep wouldn’t work as it’s an actual dependents.

The goal of @types/web is to exactly not require the DOM environment types.
It’s a bit alpha. So if you have a better solution, feel free to send a PR to improve things.

Answer 2 · 2022-07-24T14:40:16.000Z

Hi @wooorm,

Thank you for the quick reply!

I get that you want to be in control of which version of the DOM types are used and that @types/web is the way to do so.

I agree that it is debatable, whether @types/whatever should be in dependencies instead of devDependencies at all, considering @types are only necessary at dev time and have no runtime effect whatsoever.

Considering that using @types/web as a dependency is basically forcing the users of your library to switch from lib: ["dom"] in tsconfig to @types/web themselves, I would propose to "downgrade" it to a devDependency (unleass you have good reasons to want to lock your DOM types to a specific @types/web version...)

What do you think?

Answer 3 · 2022-07-24T14:58:55.000Z

I get that you want to be in control of which version of the DOM types are used and that @types/web is the way to do so.

That is not really the reason. The reason is I don’t want end users to have to enable 'dom', because this project also works well in other environments that are not browsers. For example, with jsdom.

I agree that it is debatable, whether @types/whatever should be in dependencies instead of devDependencies at all, considering @types are only necessary at dev time and have no runtime effect whatsoever.

The difference is as follows, given that you develop app A, which uses my package B, and my package uses types from package C.
If B depends on C as a dev-dependency, you get TypeScript errors about the package not being there, and you have to install it in your project.
If B depends on C as a dependency, you don’t get errors. You don’t need to install my dependencies.

Similarly, if I want 'dom' in my package B, you must enable 'dom'. I don’t want that.

Considering that using @types/web as a dependency is basically forcing the users of your library to switch from lib: ["dom"] in tsconfig to @types/web themselves, I would propose to "downgrade" it to a devDependency (unleass you have good reasons to want to lock your DOM types to a specific @types/web version...)

This should not happen. You are also the first to notice a problem with this, even though I use @types/web in other (frequently used) packages too.
If it happens, that is because:
a) you have configured TypeScript wrong (e.g., perhaps you have a very old one),
b) it is a bug in @types/web, unlikely, because they publish the same types as 'dom',
c) it is a bug here, perhaps because @types/web is out of date. That is likely due to them releasing patch releases, it would be easier if they followed semver, at least with a minor version, perhaps you can raise an issue with them to do that, if you can assert that this is the case.

Answer 4 · 2023-07-11T09:47:48.000Z

@wooorm I am sorry for not responding again! I finally figured out what the problem is: it is how @types/web is used in your repository.

The docs say to install @types/web via yarn add @typescript/lib-dom@npm:@types/web. This will only result in this being added to the package.json:

  "dependencies": {
    "@typescript/lib-dom": "npm:@types/web", // this can also be tied with a specific version
    "typescript": "5.1.6"
  }

It seems it is not necessary to have @types/web as a dependency itself. If so, it would result in having two packages (@types/web and @typescript/lib-dom) both with the contents of @types/web. This again leads to duplicate identifier errors.

Am I correct with my analysis this time? If so, would you accept a PR regarding this change?

Thanks a lot!

Answer 5 · 2023-07-12T09:07:54.000Z

I don’t know if that is correct!
I think what they mention there, and what you are doing with remapping @typescript/lib-dom, is to change the entire DOM typings.
That’s exactly what we don’t want to do? 🤔 I don’t want to require DOM in lib

Answer 6 · 2023-07-13T08:32:06.000Z

Thank you for your quick response!

I think what they mention there, and what you are doing with remapping @typescript/lib-dom, is to change the entire DOM typings.

As far as I understand it, this type of remapping is the exact point of @types/web ;)

The npm page says:

@types/web is also included inside TypeScript, available as dom in the lib section and included in projects by default. By using @types/web you can lock your the web APIs used in your projects, easing the process of updating TypeScript and offering more control in your environment.

At least from the docs, it thus seems that @types/web should not be a used as a direct dependency. People who don't specifically use it, get some version anyway in the form of typescript/lib/lib.dom.d.ts (see TypeScript-DOM-Lib-Generator). People who use @types/web as described on its npm page, will have it downloaded in @typescript/lib-dom (basically redirected, the package.json of @typescript/lib-dom even says "name": "@types/web"). So @types/web is always already present in one form or the other.

Before you said:

The reason is I don’t want end users to have to enable 'dom', because this project also works well in other environments that are not browsers. For example, with jsdom.

I totally understand that! I assume though, that browser-based environments do use dom anyway (and thus either @typescript/lib-dom, which equals @types/web; or typescript/lib/lib.dom.d.ts) . And jsdom users will use the jsdom types. So I guess there is no need to have @types/web as a dependency at all.

The problem is: apparently this even leads to problems, for everyone who does use DOM. E.g this simple case leads to conflicts:

{
  "name": "test",
  "type": "module",
  "version": "0.0.1",
  "main": "build/index.js",
  "scripts": {
    "build": "tsc"
  },
  "dependencies": {},
  "devDependencies": {
    "@types/web": "^0.0.104",
    "typescript": "5.1.6"
  }
}

{
  "compilerOptions": {
    "lib": ["ES2021", "DOM"],
    "outDir": "build",
    "rootDir": "./src",
    "baseUrl": "./",
    "rootDir": "./src",
    "sourceMap": true,
    "target": "ES2021"
  },
  "include": ["./src/**/*.ts"]
}

node_modules/typescript/lib/lib.dom.d.ts:25653:5 - error TS2374: Duplicate index signature for type 'number'.

25653     [index: number]: Window;
          ~~~~~~~~~~~~~~~~~~~~~~~~


Found 100 errors in 2 files.

Errors  Files
    55  node_modules/@types/web/index.d.ts:7
    45  node_modules/typescript/lib/lib.dom.d.ts:23

Interestingly Typescript is smart enough to ignore lib.dom.d.ts when @typescript/lib-dom exists (which I guess is hardcoded as part of the lib-replacement feature). This is the reason that redirecting @types/web to @typescript/lib-dom via yarn add @typescript/lib-dom@npm:@types/web works - at least as long as @types/web is not installed by something else (e.g. rehype-mathjax).

Long story short: Whenever DOM is enabled, @types/web will lead to conflicting types.

Hope this helps! I guess the magic of how Typescript uses DOM types makes this all kinda complicated :)

Answer 7 · 2023-07-14T10:13:39.000Z

@FunkMonkey is correct here. The intended usage of @types/web is so users can swap builtin dom libs using the dependency:

{
  "devDependencies": {
    "@typescript/lib-dom": "npm:@types/web"
  }
}

Libraries should certainly not include this, as it results in clashes for TypeScript globals, as was demonstrated in #75 (comment).

The technically correct way for a library to depend on the DOM types, is to reference the dom lib:

/// <reference lib="dom" />

Often this is just omitted though.

Answer 8 · 2023-07-14T10:13:56.000Z

Hi team! I don’t know what’s up as there’s no phase label. Please add one so I know where it’s at.

Thanks,
— bb

Answer 9 · 2023-07-15T16:50:28.000Z

@remcohaszing I believe this project does not need the DOM AFAIK? MathJax apparently does, but it’s not needed for us.

The problem is: I don’t want users to enable DOM types? How to fix that?

Answer 10 · 2023-07-18T14:29:53.000Z

mathjax-full indeed requires the DOM types. As a result, rehype-mathjax would need the DOM types to build it locally. The mathjax-full types are broken though, so skipLibCheck is enabled, meaning none of this matters.

More importantly, when using rehype-mathjax, it does not need the DOM types.

Given the following files:

// tsconfig.json
{
  "compilerOptions": {
    // This explicitly excludes `dom`
    "lib": ["es5"],
    // This explicitly excludes `@types/web`
    "types": []
  }
}

// index.ts
import 'rehype-mathjax'

The following shows that neither the DOM types, nor @types/web are included

$ tsc --listFiles    
node_modules/typescript/lib/lib.es5.d.ts
node_modules/typescript/lib/lib.decorators.d.ts
node_modules/typescript/lib/lib.decorators.legacy.d.ts
node_modules/@types/unist/index.d.ts
node_modules/vfile-message/lib/index.d.ts
node_modules/vfile-message/index.d.ts
node_modules/vfile/lib/minurl.shared.d.ts
node_modules/vfile/lib/index.d.ts
node_modules/vfile/index.d.ts
node_modules/unified/index.d.ts
node_modules/@types/hast/index.d.ts
node_modules/rehype-mathjax/lib/create-plugin.d.ts
node_modules/rehype-mathjax/svg.d.ts
node_modules/rehype-mathjax/index.d.ts
index.ts

However, many projects do need the dom types and don’t define "types" to exclude @types/web, so when we change tsconfig.json:

// tsconfig.json
{}

And when we list the files again, we see a lot of errors indicating conflict between @types/web and the builtin dom types

$ tsc --listFiles

# Lots of errors here truncated for brevity.

node_modules/typescript/lib/lib.d.ts
node_modules/typescript/lib/lib.es5.d.ts
node_modules/typescript/lib/lib.dom.d.ts
node_modules/typescript/lib/lib.webworker.importscripts.d.ts
node_modules/typescript/lib/lib.scripthost.d.ts
node_modules/typescript/lib/lib.decorators.d.ts
node_modules/typescript/lib/lib.decorators.legacy.d.ts
node_modules/@types/unist/index.d.ts
node_modules/vfile-message/lib/index.d.ts
node_modules/vfile-message/index.d.ts
node_modules/vfile/lib/minurl.shared.d.ts
node_modules/vfile/lib/index.d.ts
node_modules/vfile/index.d.ts
node_modules/unified/index.d.ts
node_modules/@types/hast/index.d.ts
node_modules/rehype-mathjax/lib/create-plugin.d.ts
node_modules/rehype-mathjax/svg.d.ts
node_modules/rehype-mathjax/index.d.ts
index.ts
node_modules/@types/mathjax/index.d.ts
node_modules/@types/web/iterable.d.ts
node_modules/@types/web/index.d.ts

Found 1268 errors in 3 files.

Errors  Files
  1011  node_modules/@types/web/index.d.ts:7
   204  node_modules/@types/web/iterable.d.ts:6
    53  node_modules/typescript/lib/lib.dom.d.ts:23

So really having @types/web as a dependency causes a lot of conflicts and rehype-mathjax doesn’t even use it.

Answer 11 · 2023-07-18T15:23:13.000Z

I’m guessing something changed over the years because I am pretty sure stuff failed before

Answer 12 · 2023-07-18T15:27:05.000Z

released, thanks y’all!

Answer 13 · 2023-07-18T15:29:12.000Z

Thank you @wooorm and @remcohaszing! :)