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

Fix all breaking todo comments for 3.0 #3907

Merged
merged 7 commits into from Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions packages/@uppy/aws-s3-multipart/src/index.js
Expand Up @@ -58,11 +58,6 @@ export default class AwsS3Multipart extends BasePlugin {

[Symbol.for('uppy test: getClient')] () { return this.#client }

// TODO: remove getter and setter for #client on the next major release
get client () { return this.#client }

set client (client) { this.#client = client }

/**
* Clean up all references for a file's upload: the MultipartUploader instance,
* any events related to the file, and the Companion WebSocket connection.
Expand Down
2 changes: 0 additions & 2 deletions packages/@uppy/companion/src/server/controllers/url.js
Expand Up @@ -93,7 +93,6 @@ const meta = async (req, res) => {
return res.json(urlMeta)
} catch (err) {
logger.error(err, 'controller.url.meta.error', req.id)
// @todo send more meaningful error message and status code to client if possible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we give up on sending a more meaningful error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to expose information to the client: #3907 (comment)

return res.status(err.status || 500).json({ message: 'failed to fetch URL metadata' })
}
}
Expand Down Expand Up @@ -125,7 +124,6 @@ const get = async (req, res) => {

function onUnhandledError (err) {
logger.error(err, 'controller.url.error', req.id)
// @todo send more meaningful error message and status code to client if possible
res.status(err.status || 500).json({ message: 'failed to fetch URL metadata' })
}

Expand Down
2 changes: 0 additions & 2 deletions packages/@uppy/core/src/BasePlugin.js
Expand Up @@ -75,8 +75,6 @@ export default class BasePlugin {
throw new Error('Extend the render method to add your plugin to a DOM element')
}

// TODO: remove in the next major version. It's not feasible to
// try to use plugins with other frameworks.
// eslint-disable-next-line class-methods-use-this
update () {}

Expand Down
16 changes: 1 addition & 15 deletions packages/@uppy/core/src/Restricter.js
@@ -1,5 +1,4 @@
/* eslint-disable max-classes-per-file, class-methods-use-this */
/* global AggregateError */
import prettierBytes from '@transloadit/prettier-bytes'
import match from 'mime-match'

Expand All @@ -17,17 +16,6 @@ class RestrictionError extends Error {
isRestriction = true
}

if (typeof AggregateError === 'undefined') {
// eslint-disable-next-line no-global-assign
// TODO: remove this "polyfill" in the next major.
globalThis.AggregateError = class AggregateError extends Error {
constructor (errors, message) {
super(message)
this.errors = errors
}
}
}

class Restricter {
constructor (getOpts, i18n) {
this.i18n = i18n
Expand Down Expand Up @@ -108,12 +96,10 @@ class Restricter {
getMissingRequiredMetaFields (file) {
const error = new RestrictionError(this.i18n('missingRequiredMetaFieldOnFile', { fileName: file.name }))
const { requiredMetaFields } = this.getOpts().restrictions
// TODO: migrate to Object.hasOwn in the next major.
const own = Object.prototype.hasOwnProperty
const missingFields = []

for (const field of requiredMetaFields) {
if (!own.call(file.meta, field) || file.meta[field] === '') {
if (!Object.hasOwn(file.meta, field) || file.meta[field] === '') {
missingFields.push(field)
}
}
Expand Down
5 changes: 0 additions & 5 deletions packages/@uppy/core/src/Uppy.js
Expand Up @@ -843,11 +843,6 @@ class Uppy {
return this.#runUpload(uploadID)
}

// todo remove in next major. what is the point of the reset method when we have cancelAll or vice versa?
reset (...args) {
this.cancelAll(...args)
}

logout () {
this.iteratePlugins(plugin => {
if (plugin.provider && plugin.provider.logout) {
Expand Down
11 changes: 6 additions & 5 deletions packages/@uppy/core/src/Uppy.test.js
Expand Up @@ -6,6 +6,7 @@ import fs from 'node:fs'
import prettierBytes from '@transloadit/prettier-bytes'
import Core from '../lib/index.js'
import UIPlugin from '../lib/UIPlugin.js'
import { debugLogger } from '../lib/loggers.js'
import AcquirerPlugin1 from './mocks/acquirerPlugin1.js'
import AcquirerPlugin2 from './mocks/acquirerPlugin2.js'
import InvalidPlugin from './mocks/invalidPlugin.js'
Expand Down Expand Up @@ -204,7 +205,7 @@ describe('src/Core', () => {
})
})

it('should reset when the reset method is called', () => {
it('should cancel all when the `cancelAll` method is called', () => {
// use DeepFrozenStore in some tests to make sure we are not mutating things
const core = new Core({
store: DeepFrozenStore(),
Expand All @@ -216,7 +217,7 @@ describe('src/Core', () => {
core.on('state-update', coreStateUpdateEventMock)
core.setState({ foo: 'bar', totalProgress: 30 })

core.reset()
core.cancelAll()

expect(coreCancelEventMock).toHaveBeenCalledWith({ reason: 'user' }, undefined, undefined, undefined, undefined, undefined)
expect(coreStateUpdateEventMock.mock.calls.length).toEqual(2)
Expand Down Expand Up @@ -1071,7 +1072,7 @@ describe('src/Core', () => {
)
})

it('allows new files again with allowMultipleUploadBatches: false after reset() was called', async () => {
it('allows new files again with allowMultipleUploadBatches: false after cancelAll() was called', async () => {
const core = new Core({ allowMultipleUploadBatches: false })

core.addFile({
Expand All @@ -1082,7 +1083,7 @@ describe('src/Core', () => {
})
await expect(core.upload()).resolves.toBeDefined()

core.reset()
core.cancelAll()

core.addFile({
source: 'jest',
Expand Down Expand Up @@ -2135,7 +2136,7 @@ describe('src/Core', () => {
console.error = jest.fn()

const core = new Core({
logger: Core.debugLogger,
logger: debugLogger,
})

core.log('test test')
Expand Down
17 changes: 1 addition & 16 deletions packages/@uppy/core/src/index.js
@@ -1,20 +1,5 @@
export { default } from './Uppy.js'
export { default as Uppy } from './Uppy.js'
export { default as UIPlugin } from './UIPlugin.js'
export { default as BasePlugin } from './BasePlugin.js'
export { debugLogger } from './loggers.js'

// TODO: remove all the following in the next major
/* eslint-disable import/first */
import Uppy from './Uppy.js'
import UIPlugin from './UIPlugin.js'
import BasePlugin from './BasePlugin.js'
import { debugLogger } from './loggers.js'

// Backward compatibility: we want those to keep being accessible as static
// properties of `Uppy` to avoid a breaking change.
Uppy.Uppy = Uppy
Uppy.UIPlugin = UIPlugin
Uppy.BasePlugin = BasePlugin
Uppy.debugLogger = debugLogger
Murderlon marked this conversation as resolved.
Show resolved Hide resolved

export { Uppy }
4 changes: 0 additions & 4 deletions packages/@uppy/core/types/index.d.ts
Expand Up @@ -217,8 +217,6 @@ export type ErrorCallback = (error: Error) => void;
export type UploadErrorCallback<TMeta> = (file: UppyFile<TMeta> | undefined, error: Error, response?: ErrorResponse) => void;
export type UploadRetryCallback = (fileID: string) => void;
export type RetryAllCallback = (fileIDs: string[]) => void;

// TODO: reverse the order in the next major version
export type RestrictionFailedCallback<TMeta> = (file: UppyFile<TMeta> | undefined, error: Error) => void;

export interface UppyEventMap<TMeta = Record<string, unknown>> {
Expand Down Expand Up @@ -349,8 +347,6 @@ export class Uppy {
fileID: string
): Promise<UploadResult<TMeta>>

reset(): void

getID(): string

use<TOptions, TInstance extends UIPlugin | BasePlugin<TOptions>>(
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/dashboard/src/Dashboard.jsx
Expand Up @@ -73,7 +73,7 @@ export default class Dashboard extends UIPlugin {
hidePauseResumeButton: false,
hideProgressAfterFinish: false,
doneButtonHandler: () => {
this.uppy.reset()
this.uppy.cancelAll()
this.requestCloseModal()
},
note: null,
Expand Down
6 changes: 2 additions & 4 deletions packages/@uppy/react/src/Dashboard.js
Expand Up @@ -19,7 +19,7 @@ class Dashboard extends Component {
if (prevProps.uppy !== this.props.uppy) {
this.uninstallPlugin(prevProps)
this.installPlugin()
} else if (nonHtmlPropsHaveChanged(this, prevProps)) {
} else if (nonHtmlPropsHaveChanged(this.props, prevProps)) {
const options = { ...this.props, target: this.container }
delete options.uppy
this.plugin.setOptions(options)
Expand Down Expand Up @@ -50,14 +50,12 @@ class Dashboard extends Component {
}

render () {
// TODO: stop exposing `validProps` as a public property and rename it to `htmlProps`
this.validProps = getHTMLProps(this.props)
return h('div', {
className: 'uppy-Container',
ref: (container) => {
this.container = container
},
...this.validProps,
...getHTMLProps(this.props),
})
}
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@uppy/react/src/DashboardModal.js
Expand Up @@ -20,7 +20,7 @@ class DashboardModal extends Component {
if (prevProps.uppy !== uppy) {
this.uninstallPlugin(prevProps)
this.installPlugin()
} else if (nonHtmlPropsHaveChanged(this, prevProps)) {
} else if (nonHtmlPropsHaveChanged(this.props, prevProps)) {
const options = { ...this.props, onRequestCloseModal: onRequestClose }
delete options.uppy
this.plugin.setOptions(options)
Expand Down Expand Up @@ -135,14 +135,12 @@ class DashboardModal extends Component {
}

render () {
// TODO: stop exposing `validProps` as a public property and rename it to `htmlProps`
this.validProps = getHTMLProps(this.props)
return h('div', {
className: 'uppy-Container',
ref: (container) => {
this.container = container
},
...this.validProps,
...getHTMLProps(this.props),
})
}
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@uppy/react/src/DragDrop.js
Expand Up @@ -20,7 +20,7 @@ class DragDrop extends Component {
if (prevProps.uppy !== this.props.uppy) {
this.uninstallPlugin(prevProps)
this.installPlugin()
} else if (nonHtmlPropsHaveChanged(this, prevProps)) {
} else if (nonHtmlPropsHaveChanged(this.props, prevProps)) {
const options = { ...this.props, target: this.container }
delete options.uppy
this.plugin.setOptions(options)
Expand Down Expand Up @@ -63,14 +63,12 @@ class DragDrop extends Component {
}

render () {
// TODO: stop exposing `validProps` as a public property and rename it to `htmlProps`
this.validProps = getHTMLProps(this.props)
return h('div', {
className: 'uppy-Container',
ref: (container) => {
this.container = container
},
...this.validProps,
...getHTMLProps(this.props),
})
}
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@uppy/react/src/ProgressBar.js
Expand Up @@ -19,7 +19,7 @@ class ProgressBar extends Component {
if (prevProps.uppy !== this.props.uppy) {
this.uninstallPlugin(prevProps)
this.installPlugin()
} else if (nonHtmlPropsHaveChanged(this, prevProps)) {
} else if (nonHtmlPropsHaveChanged(this.props, prevProps)) {
const options = { ...this.props, target: this.container }
delete options.uppy
this.plugin.setOptions(options)
Expand Down Expand Up @@ -52,14 +52,12 @@ class ProgressBar extends Component {
}

render () {
// TODO: stop exposing `validProps` as a public property and rename it to `htmlProps`
this.validProps = getHTMLProps(this.props)
return h('div', {
className: 'uppy-Container',
ref: (container) => {
this.container = container
},
...this.validProps,
...getHTMLProps(this.props),
})
}
}
Expand Down
6 changes: 2 additions & 4 deletions packages/@uppy/react/src/StatusBar.js
Expand Up @@ -20,7 +20,7 @@ class StatusBar extends Component {
if (prevProps.uppy !== this.props.uppy) {
this.uninstallPlugin(prevProps)
this.installPlugin()
} else if (nonHtmlPropsHaveChanged(this, prevProps)) {
} else if (nonHtmlPropsHaveChanged(this.props, prevProps)) {
const options = { ...this.props, target: this.container }
delete options.uppy
this.plugin.setOptions(options)
Expand Down Expand Up @@ -67,14 +67,12 @@ class StatusBar extends Component {
}

render () {
// TODO: stop exposing `validProps` as a public property and rename it to `htmlProps`
this.validProps = getHTMLProps(this.props)
return h('div', {
className: 'uppy-Container',
ref: (container) => {
this.container = container
},
...this.validProps,
...getHTMLProps(this.props),
})
}
}
Expand Down
9 changes: 2 additions & 7 deletions packages/@uppy/react/src/nonHtmlPropsHaveChanged.js
@@ -1,10 +1,5 @@
'use strict'

// TODO: replace with `Object.hasOwn` when dropping support for older browsers.
const hasOwn = (obj, key) => Object.prototype.hasOwnProperty.call(obj, key)

export default function nonHtmlPropsHaveChanged (component, prevProps) {
return Object.keys(component.props)
// TODO: replace `validProps` with an exported `Symbol('htmlProps')`.
.some(key => !hasOwn(component.validProps, key) && component.props[key] !== prevProps[key])
export default function nonHtmlPropsHaveChanged (props, prevProps) {
return Object.keys(props).some(key => !Object.hasOwn(props, key) && props[key] !== prevProps[key])
}
2 changes: 1 addition & 1 deletion packages/@uppy/redux-dev-tools/src/index.js
Expand Up @@ -40,7 +40,7 @@ export default class ReduxDevTools extends UIPlugin {
// Implement monitors actions
switch (message.payload.type) {
case 'RESET':
this.uppy.reset()
this.uppy.cancelAll()
return
case 'IMPORT_STATE': {
const { computedStates } = message.payload.nextLiftedState
Expand Down
8 changes: 0 additions & 8 deletions packages/@uppy/store-redux/src/index.js
Expand Up @@ -90,11 +90,3 @@ export function middleware () {
}

export default ReduxStore

// Backward compatibility: we want these to keep being available as static
// properties of `ReduxStore` to avoid a breaking change.
// TODO: remove these in the next semver-major.
ReduxStore.ReduxStore = ReduxStore
ReduxStore.STATE_UPDATE = STATE_UPDATE
ReduxStore.reducer = reducer
ReduxStore.middleware = middleware
2 changes: 1 addition & 1 deletion packages/@uppy/store-redux/src/index.test.js
Expand Up @@ -10,7 +10,7 @@ describe('ReduxStore', () => {

it('can be created with named or default import', () => {
const r = createStore()
let store = new ReduxStore.ReduxStore({ store: r })
let store = new ReduxStore({ store: r })
expect(typeof store).toBe('object')
store = new ReduxStore({ store: r })
expect(typeof store).toBe('object')
Expand Down
12 changes: 4 additions & 8 deletions packages/@uppy/thumbnail-generator/src/index.js
Expand Up @@ -162,9 +162,9 @@ export default class ThumbnailGenerator extends UIPlugin {
return Promise.all([onload, orientationPromise])
.then(([image, orientation]) => {
const dimensions = this.getProportionalDimensions(image, targetWidth, targetHeight, orientation.deg)
const rotatedImage = this.rotateImage(image, orientation)
const rotatedImage = rotateImage(image, orientation)
const resizedImage = this.resizeImage(rotatedImage, dimensions.width, dimensions.height)
return this.canvasToBlob(resizedImage, this.thumbnailType, 80)
return canvasToBlob(resizedImage, this.thumbnailType, 80)
})
.then(blob => {
return URL.createObjectURL(blob)
Expand Down Expand Up @@ -208,11 +208,12 @@ export default class ThumbnailGenerator extends UIPlugin {
*
* Returns a Canvas with the resized image on it.
*/
// eslint-disable-next-line class-methods-use-this
resizeImage (image, targetWidth, targetHeight) {
// Resizing in steps refactored to use a solution from
// https://blog.uploadcare.com/image-resize-in-browsers-is-broken-e38eed08df01

let img = this.protect(image)
let img = protect(image)

let steps = Math.ceil(Math.log2(img.width / targetWidth))
if (steps < 1) {
Expand Down Expand Up @@ -398,8 +399,3 @@ export default class ThumbnailGenerator extends UIPlugin {
}
}
}

// TODO: remove these methods from the prototype in the next major.
ThumbnailGenerator.prototype.canvasToBlob = canvasToBlob
ThumbnailGenerator.prototype.protect = protect
ThumbnailGenerator.prototype.rotateImage = rotateImage