Skip to content

Fix: fix prompt-installation #572 #589

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.

Sign up for GitHub

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

Closed
duzy wants to merge 1 commit into from
Closed

Fix: fix prompt-installation #572 #589

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion bin/prompt-command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ module.exports = function promptForInstallation(packages, ...args) {
}
});
} else {
require(pathForCmd).default(...args); // eslint-disable-line
let p = require(pathForCmd), c = p[packages];
Copy link
Member

@evenstensberg evenstensberg Sep 15, 2018

Choose a reason for hiding this comment

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

Can you try to make this a bit prettier with better variable names?

Copy link
Author

@duzy duzy Sep 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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);

Copy link
Contributor

@ematipico ematipico Sep 15, 2018

Choose a reason for hiding this comment

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

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

if (typeof c !== 'function' && typeof p.default === 'function') {
c = p.default;
}
c(...args); // eslint-disable-line
}
};