Conversation
yakhinvadim
left a comment
yakhinvadim
left a comment
There was a problem hiding this comment.
Really like the simplification!
src/index.js
Outdated
| export { Exporter }; | ||
|
|
||
| export default function(deckName, template) { | ||
| export default function(deckName, template, sql) { |
There was a problem hiding this comment.
What do you think about providing parameters via object?
export default function({deckName, template, sql}) {
We now have 3 params, and 1st and 3rd are required, which is a little odd.
Also, should we provide a default value for template?
There was a problem hiding this comment.
I agree with @yakhinvadim, this does seem awkward as template is optional. I do see an advantage for using object notation for method params. This could be a good pattern that other methods could benefit from, e.g. addCard.
There was a problem hiding this comment.
Initially I didn't want to introduce breaking changes, but it totally makes sense. Thanks for insisting 👍
| - [browser usage with media attachments via <form />](examples/browser-media-file-input) | ||
| - [Node.js](examples/server) | ||
| - [Browser/webpack/asm, with ajax and input media attachments](examples/browser) | ||
| - [Browser/webpack/wasm, with ajax and input media attachments](examples/browser) |
There was a problem hiding this comment.
Should this link to examples/browser-wasm?
jamsinclair
left a comment
jamsinclair
left a comment
There was a problem hiding this comment.
Looks great! I totally agree, we should leave it up the user as to which sql.js implementation they want to use.
src/index.js
Outdated
| export { Exporter }; | ||
|
|
||
| export default function(deckName, template) { | ||
| export default function(deckName, template, sql) { |
There was a problem hiding this comment.
Should we throw an explicit error if sql argument is not defined? It would help people migrating from older code as it would fail loudly and clearly.
3 participants
No description provided.