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

Architecture #9

Merged
merged 14 commits into from Nov 19, 2015
Merged

Architecture #9

merged 14 commits into from Nov 19, 2015

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Nov 17, 2015

We can play with the draft code here: https://jsbin.com/vupurugona/edit?js,console

There are two main issues that I faced and that we need to discuss:

How to register plugins

Two ways I’ve found that are worth investigating:

The PostCSS way: Starts with postcss.plugin function, which is defined here. Function returns a function, you can add plugins like this:

postcss([
  autoprefixer({ browsers: ['last 2 versions'] }),
  postcssSVG({
    paths: [path.join(paths.srcPath, 'assets')],
    defaults: '[fill]: #000000'
  })
]);

The Markdown-It way: They went with a use function that calls the plugin with options you passed to it:

const md = require('markdown-it')({
  html: true,
  breaks: true
})
.use(require('markdown-it-anchor'), {
  slugify: util.slugifyHeaderId
});

Use of class

From what I gathered some are using classes happily, some expose the API and want the users to do let markdown = new Markdown(), while others, like PostCSS, expose the function only, which creates a new instance inside itself and uses that internally.

Todo in this PR

  • Decide on the way to include a plugin
  • Decide on the use of classes
  • Create an example Instagram plugin (doesn't have to work, just for the structure)
  • Move example plugin to own file
  • Cement the consensus of Modals by default? #19 into ARCHITECTURE.md
  • Restore LICENSE
  • Restore README.md
  • Decide on file extension .js / .es6 Should we use .es6 extensions? #10

@@ -1,22 +0,0 @@
The MIT License (MIT)
Copy link
Member

Choose a reason for hiding this comment

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

License can remain MIT

@kvz
Copy link
Member

kvz commented Nov 17, 2015

I can't say I know all the pros and cons. But I like the .use() way of adding plugins. It seems to me it's a bit more expressive, and also allows you to more easily add plugins conditionally. Like:

if (true) {
  transloadit.use(require('transloadit-client-instagram'), {
    only_sepia: false,
  });
}

@hedgerh
Copy link
Contributor

hedgerh commented Nov 17, 2015

i like the .use() syntax as well. how do you all feel about using chaining for some configuration?

for instance,

$('#upload-form').transloadit({
  modal: false,
  onProgress: function(bytesReceived, bytesExpected) {
    // render your own progress bar!
    $('#progress')
      .text((bytesReceived / bytesExpected * 100).toFixed(2) + '%');
  },
  onError: function(assembly) {
    alert(assembly.error + ': ' + Assembly.message);
  }
});

could be:

transloadit
  .set({ modal: false })
  .on('progress', handleProgress)
  .on('error', handleError);

@kvz
Copy link
Member

kvz commented Nov 17, 2015

how do you all feel about using chaining for some configuration?

+1

@arturi
Copy link
Contributor Author

arturi commented Nov 17, 2015

Here is what Andrey (the creator of PostCSS) replied to me:

Если ты делаешь для JS, то используй API markdown-it
В PostCSS пользователи сами должны вызывать функцию плагина, но они постоянно забывают это делать. В итоге у нас куча магии, соды это избежать

@arturi
Copy link
Contributor Author

arturi commented Nov 17, 2015

Ha-ha, ok, so he says “if you are building a JS SDK, use the API from markdown-it [meaning use], in PostCSS users have to manually call the plugin function, which they frequently forget to do”.

@kvz
Copy link
Member

kvz commented Nov 17, 2015

use the API from markdown-it [meaning use]

Okay that settles it : ) Please cement this choice in ARCHITECTURE.md?

@tim-kos
Copy link
Member

tim-kos commented Nov 18, 2015

+1 on .use() and +1 on chaining.

As for the class discussion: I am happy to only expose one function and not classes. I have read through other JS SDKs of competitors and I liked how easy they were to use with just one function exposed.

And I think with .use() basically all functionality can be easily added via a plugin, so we don't need to expose several classes. For example even themes could be used with .use:

transloadit
  .set({ modal: false, wait: true })
  .use('dropbox', {some: 'config'})
  .use('instagram', {some: 'config'})
  .use('theme_sunset', {
     some: 'overwritten_color',
     custom_css: 'some_css_string'
   })
  .on('progress', handleProgress)
  .on('error', handleError);

@arturi
Copy link
Contributor Author

arturi commented Nov 18, 2015

All right. Not sure we should mix themes and plugins, though.

@arturi
Copy link
Contributor Author

arturi commented Nov 18, 2015

A question. If we do .use('dropbox', {some: 'config'}), that means the plugin has to be made available somehow separately, or already be attached to the main instance. I proposed .use(dropbox, {some: 'config'})dropbox is a plugin function.

@hedgerh
Copy link
Contributor

hedgerh commented Nov 18, 2015

+1 for plugin functions. it would save us from needing to update the core any time we added a new plugin. also easier for third-party plugins to be written by the community. I think we should bundle the main plugins with the client. it could be used:

import Transloadit from 'transloadit-client'
import { dropbox, instagram, dragdrop, webcam } from 'transloadit-client/plugins'

// or to import all of them
import { * as plugins } from 'transloadit/plugins'

@hedgerh hedgerh closed this Nov 18, 2015
@hedgerh hedgerh reopened this Nov 18, 2015
@arturi
Copy link
Contributor Author

arturi commented Nov 18, 2015

What is happening.

@kvz
Copy link
Member

kvz commented Nov 18, 2015

I think we should bundle the main plugins with the client. it could be used

+1 as well on this

All right. Not sure we should mix themes and plugins, though.

What about: .useTheme(default), .useAcquire(dropbox) .useModify(cropping) to distinct in different plugin types?

@arturi
Copy link
Contributor Author

arturi commented Nov 18, 2015

What about: .useTheme(default), .useAcquire(dropbox) .useModify(cropping) to distinct in different plugin types?

Is theme a plugin tough? Won’t it just be a styles file?

But yeah, we should probably distinguish between upload methods and crop or other functionality.

@kvz
Copy link
Member

kvz commented Nov 18, 2015

Is theme a plugin tough? Won’t it just be a styles file?

Good question. But I would like to ship a few styles. And maybe allow people to set just the primary color. This is how other companies (like intercom) currently provide customization - and it works pretty well actually.

It should of course be possible to roll your own css - but I don't think anything stands in the way of that.

But yeah, we should probably distinguish between upload methods and crop or other functionality.

Thinking about this more - and especially after #19 (comment), I'm currently leaning towards only having a .use(), and the plugin having a type property to signal if it is a theme, acquire, etc

@kvz kvz mentioned this pull request Nov 18, 2015
@kvz
Copy link
Member

kvz commented Nov 19, 2015

Nice work @arturi! I'll fork off the remaining subtodos and close this massive one! Smaller more focused iterations are possible after this!

kvz added a commit that referenced this pull request Nov 19, 2015
@kvz kvz merged commit 80e2e2a into master Nov 19, 2015
@kvz kvz deleted the architecture branch November 19, 2015 17:24
This was referenced Nov 19, 2015
DimaKogut added a commit to DimaKogut/uppy that referenced this pull request Feb 23, 2022
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.

None yet

4 participants