Fix: fix prompt-installation #572#589
Conversation
the strange things in this fix is about packages -- if it's not indicating the sub-command or a list of commands?
evenstensberg
left a comment
There was a problem hiding this comment.
Thanks! Mind filling in the issue template with a better description? :
| }); | ||
| } else { | ||
| require(pathForCmd).default(...args); // eslint-disable-line | ||
| let p = require(pathForCmd), c = p[packages]; |
There was a problem hiding this comment.
Can you try to make this a bit prettier with better variable names?
There was a problem hiding this comment.
@ev1stensberg Hi Even, you might try refining the name as wish, I'd like to present this patch for discussion as I'm NOT quite certain about packages -- as the name literally suggested, it might have multiple package names? The other possible fix might be changing the @webpack-cli/xxx modules, e.g.
function serve() {
blah(...)
blah(...)
}
export abc;
export xyz;
export default serve;as webpack-cli seems to expect any @webpack-cli/xxx to offer a default export as a function, which is rational. But webpack-cli deserves a type check.
// pseudo
let module = required(pathForCmd), func = module.default;
if (typeof func !== 'function') throw new exception('opps, @webpack-cli/'+packages+' not meeting the spec');
func(...args);There was a problem hiding this comment.
Yes in theory the package that we want to load should export a default function and I think bombing an error is fine, in order to dictate a standard. Your pseudo solution looks fine
|
Can you produce a PR that contains full naming conventions and try to make it so that the tests pass? |
evenstensberg
left a comment
There was a problem hiding this comment.
Hi @duzy, how's it going? Do you need help?
|
Hi @evenstensberg , I'm working on other non-js projects. Just can't follow up in time. But yes, my JS project is using this patch and working so far. I will keep following up this thread. |
|
No worries |
the strange things in this fix is about packages -- if it's not indicating the sub-command or a list of commands?
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Fixes #572