[pull] main from enyo:main - #11
Merged
Merged
Conversation
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.
Convert the library to TypeScript and ship the types
Every entry says "Thanks @enyo!", because that name is the commit author and almost every commit here is mine. Thanking myself on each line is noise, and worse, it spends the word on the wrong thing: it says nothing about who found the problem or who wrote the fix. disableThanks drops that clause and keeps what is worth having -- the links to the pull request and the commit. Crediting somebody is now something you write into the changeset, in a sentence that can say what they actually did: Send a single chunk for zero byte files instead of hanging. Reported and fixed by [@Forceu](https://github.com/Forceu). The changelog-github option is used rather than its `template`, which reaches the same place but is marked experimental and documented as able to change in a patch release. Entries already written are left alone; they are published history.
Version packages
The publish of 6.2.1 succeeded on npm and then failed attaching dist.zip,
with "release not found" against v6.2.1.
Changesets only prefixes a tag with v for a single-package repository. In
@changesets/cli, buildGitTag returns `v${version}` when the workspace tool is
"root" and `${name}@${version}` otherwise, so becoming a pnpm workspace moved
the tags to dropzone@<version> without anything here saying so. This step
kept asking for v<version>, which had never been created.
It now uses the name and version the action reports, which is the tag
changesets built from the same two values.
The two stylesheets used sass for nesting, a handful of variables and one math.div call. None of that needs a compiler: the sources are now the flattened CSS sass was producing anyway, and sass is gone from the package. Flattened rather than nested. Native CSS nesting only reached browsers in 2023, well above the range a library targeting es2017 and supporting everything except Internet Explorer can assume. Lightning CSS minifies them. It is the more thorough of the minifiers tried: it drops a `background: #999` fallback that no browser in range needs, shortens `transition: opacity .3s ease` to `.3s` because ease is the default, and removes a dead `display: block` that was being overridden on the very next line -- which is in the scss too, and is removed at the source here. The supported browsers are declared once, as a browserslist field in package.json, and the build reads them with --browserslist rather than repeating a list of versions inside a script. That range is the one the es2017 JavaScript target implies, and it is load-bearing: given a modern range instead, Lightning CSS rewrites every rgba() into hex-with-alpha, which is newer syntax than this library claims to support. Checked by widening the field and watching the output change. The output was compared declaration by declaration against what sass produced: 175 effective declarations before and after in dropzone.css, 33 in basic.css, no difference once value-level equivalences are normalised. watch-css is removed rather than ported. It existed because sass had a --watch flag; lightningcss has none, nothing referenced the script, and a script called watch-css that does not watch is worse than no script. src/.gitignore is deleted with the stylesheets. It ignored *.css and *.map next to the sources, which was harmless while they were sass -- and silently swallowed both new files the moment .css became the source, including hiding them from the formatter.
Drop the sass dependency and ship plain CSS
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )