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

@uppy/dashboard: auto discover and install plugins without target #4343

Merged
merged 18 commits into from
Oct 12, 2023

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Mar 3, 2023

Why this change?

  • People are confused by having to use a plugin without a target and then listing it in Dashboard’s plugin=['Audio', 'GoogleDrive'...] array.
  • RemoteSources currently don't work with @uppy/react. RemoteSources sets target: Dashboard by default, this PR removes it.
  • Reduces boilerplate.
  • Order of plugins doesn’t matter: can use Dashboard plugins before Dashboard.

This idea was discussed when React components were first introduced: #170 (comment).

It seems that discoverProviderPlugins() doesn’t work currently, or I can’t understand how it worked.

With this PR you can use target manually still (the old way), or let the Dashboard auto-mount plugins (the new way).

Before:

uppy.use(Dashboard)
  .use(Webcam, { target: Dashboard })
  .use(ImageEditor, { target: Dashboard })
  .use(ScreenCapture, { target: Dashboard })

After:

uppy.use(ScreenCapture)
  .use(Dashboard)
  .use(Webcam)
  .use(ImageEditor)

Todo:

  • check unmount

@arturi arturi requested review from mifi and Murderlon March 3, 2023 16:59
@arturi arturi changed the title @uppy/dashboard: auto install @uppy/dashboard: auto discover and install plugins without target Mar 3, 2023
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

nice stuff! requires a major release, right?

packages/@uppy/dashboard/src/Dashboard.jsx Show resolved Hide resolved
packages/@uppy/dashboard/src/Dashboard.jsx Outdated Show resolved Hide resolved
@@ -38,7 +37,6 @@ export default class RemoteSources extends BasePlugin {

const defaultOptions = {
sources: Object.keys(availablePlugins),
target: Dashboard,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a breaking change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for RemoteSources unfortunately. But not for the Dashboard. But if you update RemoteSources and not the Dashboard, it will stop working, true.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make that opt-in somehow? Maybe by adding an option ignoreTarget: true, and we would remove both options in the next major, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Hope we can figure out something without adding more options. Either by letting plugins do breaking changes whenever they want or by skipping this feature in remote sources for now.

packages/@uppy/core/src/Uppy.js Show resolved Hide resolved
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Nice experimentation! I like the idea but have some questions:

  • This forcibly sets the target? What if you have 3 plugins you want on dashboard and 1 elsewhere, will the last one be forced added too? How do you prevent this from happening?
  • Does this deprecate the plugins array?

After discussions I think we would need tests too:

  • e2e test in every framework
  • unit tests for different cases (such as in my first question)

And of course docs :)

packages/@uppy/dashboard/src/Dashboard.jsx Outdated Show resolved Hide resolved
@arturi
Copy link
Contributor Author

arturi commented Mar 6, 2023

This forcibly sets the target? What if you have 3 plugins you want on dashboard and 1 elsewhere, will the last one be forced added too?

No, this only sets the target id you didn‘t specify a target:
if (plugin && !plugin.opts?.target && typesAllowed.includes(plugin.type))

Does this deprecate the plugins array?

No, trying to keep it.

e2e test in every framework

Sounds like an overkill just for this PR / change. We could make sure existing tests account for some plugins with no target.

@Murderlon
Copy link
Member

Sounds like an overkill just for this PR / change. We could make sure existing tests account for some plugins with no target.

Since we shippped remote sources without testing frameworks I'd say it's quite important. But don't worry, it's quite trivial to add!

Add the plugin here:

const uppyDashboard = new Uppy({ id: 'dashboard' })

Check whether it rendered correctly, similar to this:

it('should render Dashboard in React and show thumbnails', () => {
cy.get('@dashboard-input').selectFile(['cypress/fixtures/images/cat.jpg', 'cypress/fixtures/images/traffic.jpg'], { force:true })
cy.get('#dashboard .uppy-Dashboard-Item-previewImg')
.should('have.length', 2)
.each((element) => expect(element).attr('src').to.include('blob:'))
})

Can be done in couple minutes.

@arturi
Copy link
Contributor Author

arturi commented Mar 6, 2023

Can be done in couple minutes.

Thanks for the example! But what about Vue, Angular, Svelte, you mentioned every framework? Seems like that one is only React? We do have Vue too.

@Murderlon
Copy link
Member

We have the same setup for Vue so that's also a quick addition. Perhaps we can leave it at that for now. Just good to know whether it works in something else than React too.

@arturi arturi requested a review from Murderlon October 5, 2023 13:56
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Code LGTM. Have we tested what happens when you use multiple Uppy instances and multiple Dashboards on the page? Or one drag-drop and one dashboard which share a plugin?

@arturi
Copy link
Contributor Author

arturi commented Oct 12, 2023

Checked with multiple instances, plugins were installed to the correct instances.

@arturi arturi merged commit 8b25208 into main Oct 12, 2023
18 checks passed
@arturi arturi deleted the dashboard-auto-install branch October 12, 2023 22:19
Murderlon added a commit that referenced this pull request Oct 16, 2023
* main:
  @uppy/vue: export FileInput (#4736)
  examples: update `server.py` (#4732)
  @uppy/aws-s3-multipart: fix `uploadURL` when using `PUT` (#4701)
  @uppy/dashboard: auto discover and install plugins without target (#4343)
  e2e: upgrade Cypress (#4731)
  @uppy/core: mark the package as side-effect free (#4730)
  Bump postcss from 8.4.16 to 8.4.31 (#4723)
  meta: test with the latest versions of Node.js (#4729)
github-actions bot added a commit that referenced this pull request Oct 20, 2023
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/angular             |   0.6.1 | @uppy/progress-bar        |   3.0.4 |
| @uppy/audio               |   1.1.4 | @uppy/provider-views      |   3.6.0 |
| @uppy/aws-s3              |   3.4.0 | @uppy/react               |   3.1.4 |
| @uppy/aws-s3-multipart    |   3.8.0 | @uppy/remote-sources      |   1.1.0 |
| @uppy/box                 |   2.1.4 | @uppy/screen-capture      |   3.1.3 |
| @uppy/companion           |  4.10.0 | @uppy/status-bar          |   3.2.5 |
| @uppy/companion-client    |   3.5.0 | @uppy/store-default       |   3.0.5 |
| @uppy/compressor          |   1.0.5 | @uppy/store-redux         |   3.0.5 |
| @uppy/core                |   3.6.0 | @uppy/svelte              |   3.1.1 |
| @uppy/dashboard           |   3.6.0 | @uppy/thumbnail-generator |   3.0.6 |
| @uppy/drop-target         |   2.0.2 | @uppy/transloadit         |   3.3.2 |
| @uppy/dropbox             |   3.1.4 | @uppy/tus                 |   3.3.2 |
| @uppy/facebook            |   3.1.3 | @uppy/unsplash            |   3.2.3 |
| @uppy/file-input          |   3.0.4 | @uppy/url                 |   3.3.4 |
| @uppy/form                |   3.0.3 | @uppy/utils               |   5.5.2 |
| @uppy/golden-retriever    |   3.1.1 | @uppy/vue                 |   1.1.0 |
| @uppy/google-drive        |   3.3.0 | @uppy/webcam              |   3.3.4 |
| @uppy/image-editor        |   2.2.2 | @uppy/xhr-upload          |   3.4.2 |
| @uppy/informer            |   3.0.4 | @uppy/zoom                |   2.1.3 |
| @uppy/instagram           |   3.1.3 | uppy                      |  3.18.0 |
| @uppy/onedrive            |   3.1.4 |                           |         |

- @uppy/aws-s3-multipart: fix `TypeError` (Antoine du Hamel / #4748)
- meta: Bump tough-cookie from 4.1.2 to 4.1.3 (dependabot[bot] / #4750)
- meta: example: simplify code by using built-in `throwIfAborted` (Antoine du Hamel / #4749)
- @uppy/aws-s3-multipart: pass `signal` as separate arg for backward compat (Antoine du Hamel / #4746)
- meta: fix TS integration (Antoine du Hamel / #4741)
- meta: fix js2ts check (Antoine du Hamel)
- meta: add support for TypeScript plugins (Antoine du Hamel / #4640)
- @uppy/vue: export FileInput (mdxiaohu / #4736)
- meta: examples: update `server.py` (codehero7386 / #4732)
- @uppy/aws-s3-multipart: fix `uploadURL` when using `PUT` (Antoine du Hamel / #4701)
- @uppy/dashboard: auto discover and install plugins without target (Artur Paikin / #4343)
- meta: e2e: upgrade Cypress (Antoine du Hamel / #4731)
- @uppy/core: mark the package as side-effect free (Antoine du Hamel / #4730)
- meta: Bump postcss from 8.4.16 to 8.4.31 (dependabot[bot] / #4723)
- meta: test with the latest versions of Node.js (Antoine du Hamel / #4729)
- meta: e2e: update Parcel (Antoine du Hamel / #4726)
- meta: uppy: fix types (Antoine du Hamel / #4721)
- @uppy/core: type more events (Antoine du Hamel / #4719)
- @uppy/svelte: fix TS build command (Antoine du Hamel / #4720)
- @uppy/companion: Bucket fn also remote files (Mikael Finstad / #4693)
- @uppy/companion-client: fixup! Added Companion OAuth Key type (Murderlon / #4668)
- @uppy/companion-client: Added Companion OAuth Key type (Chris Pratt / #4668)
- meta: check for formatting in CI (Antoine du Hamel / #4714)
- meta: bump get-func-name from 2.0.0 to 2.0.2 (dependabot[bot] / #4709)
- meta: run Prettier on existing files (Antoine du Hamel / #4713)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants