Convert the library to TypeScript and ship the types - #2361
Merged
Merged
Conversation
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
enyo
added this pull request to stack #2364
September 12, 2026 17:56
enyo
force-pushed
the
typescript
branch
8 times, most recently
from
September 13, 2026 08:39
9704d80 to
a096094
Compare
The four source files are now TypeScript, and the build emits declarations into dist alongside the bundles. package.json gains a types field pointing at them, so the package is typed from its own source rather than from @types/dropzone, which is stuck at 5.7.9 and describes the v5 API. The types were compared against @types/dropzone to make sure this is not a step down. It covers all 52 options that package types, plus 44 it does not, and three things came out of the comparison: Options whose default is null are annotated -- `maxFiles: null as number | null` -- rather than left as bare null, which carries no type at all. Eight options were effectively `any` without this. Doing that surfaced seven places reading an option that can be absent without checking, including resize(), whose own documentation says width and height may be null while its signature did not. file.upload is a DropzoneFileUpload rather than Record<string, any>, so progress, total, bytesSent, uuid and the chunking fields are typed and a misspelled one is an error. `on` and `emit` type their listener arguments per event, which is where @types was genuinely ahead. It hand-writes 24 overloads; here the map is derived from the option handler of the same name, so it covers all 29 events and cannot drift. Unknown event names still work. Two differences are deliberate rather than regressions. @types augments the global HTMLElement with a required `dropzone` property, which is untrue of every element that is not one. And it declares a jQuery plugin behind a /// reference to @types/jquery, which pulls jQuery's types into every consumer. The options type is derived, not written: DropzoneOptions is Partial<typeof defaultOptions>, so all 96 options follow the source by construction. Neither it nor the resolved form has an index signature, and @types/dropzone does not have one either. One would accept any key at all, so `maxFileSize` -- the wrong capitalisation of `maxFilesize` -- would pass silently, and internally it was already hiding a read of this.options.fixOrientation, an option that does not exist and never did. That read was always undefined and its value is only ever tested for truthiness, so it is false now. Custom keys still survive the merge at runtime; reading one back off this.options needs a cast. The statics live in the class rather than being declared there and assigned 1,700 lines further down. That split let the two halves disagree: discover() was declared as returning void while it returns an array of dropzones, and nothing could catch it. Dropzone.version now comes from package.json. Dropzone.autoDiscover is not declared: it is documented, but nothing in 6.x reads it. initClass() is gone with it. It existed to write four properties onto the prototype, which is why they needed `declare` rather than an initialiser -- and one of them was a bug: _thumbnailQueue was a single array shared by every dropzone on the page, so two of them queued into the same list and rendered from it under one lock. They are ordinary instance fields now, with a test that fails against the old arrangement. Emitter moves to a static, since being on the prototype was only ever a way to reach it. The instance fields `version` and `defaultOptions` are removed. Neither was ever assigned or read; defaultOptions in particular claimed a reachable default that the roadmap lists as not existing. strict is on, along with noUnusedLocals, noImplicitReturns, noFallthroughCasesInSwitch, noImplicitOverride, and no unreachable code or unused labels. noUnusedParameters is deliberately not among them: the option handlers are public signatures whose parameter names document what arrives. CI runs tsc --noEmit as its own step ahead of the build. The website is checked too, which it never was: svelte-check covers the TypeScript inside .svelte files, and --threshold warning means a warning fails the job. Two more things the conversion found, both behaviour-preserving to fix: three conditions in the EXIF code used a bitwise & between two comparisons, which only works by coercing booleans to 0 and 1; and Dropzone.isValidFile assumed acceptedFiles was a string and called .split on it, though an array was already reaching it.
The website imports dropzone from the workspace, so svelte-check needs dist/dropzone.d.ts to exist. Running the check first meant it ran against a library with no declarations at all, and reported that as two errors in the website: a named import of Dropzone that "can only be imported by using a default import", and an implicit any for a rest parameter that is only implicit because the method it is passed to had no type either. Neither was about the website. Both came from the step order, which is why this only appeared once someone imported Dropzone by name. build-site.sh already builds the library as its first step, so moving the check after it costs nothing.
The website imports dropzone from the workspace, which meant nothing here could be type-checked until the library had been built: without dist/dropzone.d.ts, svelte-check reported the missing declarations as errors in the website. That is what turned a named import of Dropzone into "can only be imported by using a default import" and an ordinary rest parameter into an implicit any -- neither of which was about the website at all. main, module and types now point at src/dropzone.ts, and publishConfig points them back at dist for npm. pnpm swaps the fields when the package is packed and drops publishConfig itself, so what is published is unchanged: main dist/dropzone.js, module dist/dropzone.mjs, types dist/dropzone.d.ts. Verified by packing the tarball and reading its package.json. Two things had to be true for the source to compile under another package's tsconfig, since that is now what happens: The ambient declarations are referenced from dropzone.ts itself rather than only being listed in this package's tsconfig, so EXIF and jQuery are found by whoever is compiling it. `element` is declared rather than definitely assigned. The constructor assigns it, and under useDefineForClassFields -- which the website's config has and this one does not -- a field declaration would overwrite what Emitter provides. The blast radius of compiling the source is the workspace only: everyone installing from npm still gets the declarations, built here. The end-to-end tests and the standalone bundle still need the build, and so does dropzone/dist/dropzone.css, which the website imports. Once the stylesheet is plain CSS that import can point at src as well.
Neither README nor the documentation mentioned TypeScript at all, and the thing a TypeScript user most needs to be told is to uninstall @types/dropzone: it stopped at 5.7.9 and describes the v5 API, so it disagrees with what they are using. Both READMEs say so, and the installation page gains a TypeScript section with the two things worth knowing beyond `it just works` -- that listener arguments are inferred from the event name, and that overriding a handler gives a typed `this`. Both examples were compiled against the built declarations rather than written from memory. The links to the options source were stale twice over: they pointed at src/options.js, which moved into packages/dropzone in the monorepo change and is now .ts. All five are corrected. The line anchor in events.md is dropped rather than renumbered -- it was pointing 50 lines off already, and would go stale again with the next edit.
Trimming the section left DropzoneFile imported but no longer used: the listener's argument is inferred, which is what the example is there to show. Anyone copying it with noUnusedLocals on would have got an error out of the first thing they tried. Found by extracting the snippet from the page and compiling it under strict with noUnusedLocals. That was done by hand -- nothing in CI checks the code in the documentation, so the next example to go stale will go stale quietly.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
src/dropzone.js,options.js,emitter.jsandextend.jsare now TypeScript, the build emits declarations intodist/, andpackage.jsongains atypesfield.This matters beyond tidiness:
@types/dropzoneis stuck at5.7.9and describes the v5 API, so every v6 TypeScript user today is either untyped or actively wrongly typed.strictis ontscruns as part ofbuild, so a type error fails the build. Clearing it took 285 errors. Most were mechanical — 162 parameters without annotations — but four were structural, and those are the ones worth reviewing:this.optionsis typed as the merged result, not the user's partial input. There are now two types:DropzoneOptionsis what you pass in (everything optional),ResolvedDropzoneOptionsis what an instance holds (everything present, because the defaults have been merged). Reading an option through thePartialmade every access| undefinedfor a value that is always there, and that accounted for most of the null-safety noise.Options whose default is
nullare widened.typeof defaultOptionstypesurl: nullas exactlynull, so assigning a string to it is an error. A mapped type widens those; every other option keeps the type its default implies, which is the whole point of deriving the type rather than writing it by hand.The 14 option handlers that use
thisnow declare it. Overriding one gets a typedDropzonerather than the options literal — and it reaches the published declarations, so it types your code too:DropzoneFile.statusand.uploadare required, not optional.addFilesets both before anything reads them, so optional described a window no consumer ever sees.The options type is derived, not written
All 95 options follow
src/options.tsby construction — nothing to hand-maintain, nothing that can drift. JSDoc comes through into the declarations, so editors show the documentation on hover.DropzoneFileandDropzoneListenerare type aliases rather than interfaces, as requested. There are no interfaces left in the source.Verified from the outside
Compiling a consumer against
dist/understrict:Compiles clean — which also means the
@ts-expect-errorfired. Had the type beenany, the directive would be unused andtscwould have failed.252 tests and 3 end-to-end specs pass.
Two things the conversion found
Both behaviour-preserving:
Bitwise
&between two comparisons. Three conditions in the EXIF code read(seg[0] === 255) & (seg[1] === 225), which only works by coercing booleans to0/1. Both operands are pure comparisons, so&&is exactly equivalent.Dropzone.isValidFileassumedacceptedFileswas a string and called.spliton it. An array was already reaching it and would have thrown; it is now accepted, as the signature says.exportsis deliberately not addedIt is on the 7.0 list. Adding it now changes module resolution for every consumer, which is not a patch-level change.
typessits next tomain/moduleand is purely additive.On
src/types.d.tsYou said you saw no reason to maintain a separate
types.d.ts, and this is not that — the API description is generated. This file declares things that are not in the source at all: vite's?rawand?inlineimports, and theEXIFandjQueryglobals, which the library only touches when the page already provides them.