require extension option explicitly #13218
Conversation
|
Why was this PR necessary? Can you add a description? |
gulpfile.js
Outdated
| function declareExtension(name, version, options) { | ||
| const defaultOptions = {hasCss: false}; | ||
| const extensionKey = `${name}-${version}`; | ||
| extensions[extensionKey] = Object.assign({ | ||
| name: name, | ||
| version: version, | ||
| hasCss: hasCss, |
There was a problem hiding this comment.
Can remove this line, hasCss isn't available anymore in this scope!
gulpfile.js
Outdated
| function declareExtension(name, version, options) { | ||
| const defaultOptions = {hasCss: false}; | ||
| const extensionKey = `${name}-${version}`; | ||
| extensions[extensionKey] = Object.assign({ | ||
| name: name, |
There was a problem hiding this comment.
No requirement here, but can leverage Shorthand property names.
Object.assign({
name,
version,
}, defaultOptions, options);
|
This pull request introduces 1 alert - view on lgtm.com new alerts:
Comment posted by lgtm.com |
gulpfile.js
Outdated
| * loadPriority: ?string | ||
| * }} | ||
| */ | ||
| const ExtensionOption; |
There was a problem hiding this comment.
Looks like a leftover const definition. Mind removing?
There was a problem hiding this comment.
this is required for jscompiler typechecking (it needs a reference to an identifier)
|
@rsimha-amp The extension I'm working on adds some additional options. While reviewing how to introduce – I noticed this pattern where the @erwinmombay and I chatted about it and didn't see a reason why it should do this instead of passing a known typed argument. This PR is the result of that conversation. |
erwinmombay
force-pushed
the
normalize-declare-ext-options
branch
from
a0acebf
to
2be24ba
Compare
|
@rsimha-amp I'm just not a fan of the thisTypeOrAnotherType pattern (because javascript doesn't have method/function overloading). Unions are also OK but but this one tries to emulate method overloading instead which always turns ugly logically. (just look at the If else logic we had to remove) |
get rid of thisOrThat pattern (tries to emulate function overloading) in favor of explicit option object requirement.