heapwolf / prompt-sync

a synchronous prompt for node.js
MIT License
211 stars 42 forks source link

Fixed an issue that would cause an error. #27

Closed ghost closed 7 years ago

ghost commented 7 years ago

Changed: module.exports = create; to module.exports = create(); to fix,

TypeError: Cannot create property 'autocomplete' on string 'foo ' error.

Just a little thing!

davidmarkclements commented 7 years ago

@ozzie1998 its been a while since I've worked woth this code, but this fundamentally changes the API, and doesn't allow for config. Can you send a stack trace of the error so we can find out what's causing it?

ghost commented 7 years ago

I can do.

ghost commented 7 years ago
/Users/ozzie/Desktop/neon/node_modules/prompt-sync/index.js:19
    var autocomplete = (config.autocomplete =
                                            ^

TypeError: Cannot create property 'autocomplete' on string '> '
    at create (/Users/ozzie/Desktop/neon/node_modules/prompt-sync/index.js:19:42)
    at search (/Users/ozzie/Desktop/neon/search.js:6:15)
    at Object.<anonymous> (/Users/ozzie/Desktop/neon/search.js:25:18)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:393:7)

When prompt-sync is required like this:

var prompt = require('prompt-sync') forgetting the () on the end gets irritating.

davidmarkclements commented 7 years ago

exporting a function is a common pattern (examples: http://npm.im/express, http://npm.im/pino, https://www.npm.im/pump)

you can be more explicit to help you to remember:

var createPrompt = require('prompt-sync')
var prompt = createPrompt()

Can you let me know what code you're using to create the stack trace?

ghost commented 7 years ago
var prompt = require('prompt'); 
var foo = prompt("stuff"); 
davidmarkclements commented 7 years ago

ok I understand

you need to instantiate prompt sync first

var createPrompt = require('prompt-sync')
var prompt = createPrompt()
var foo = prompt('stuff')

You've indicated that you know this, and thought that a solution would be to just instantiate it from within the lib. However - that means prompt would be a singleton which is generally a poor design choice. It also means a user wouldn't be able to configure prompt at init time and if you changed the API so you could configure the instance, well now you have the shared state problem with the potential for clashing configs for the same singleton.

What you're trying to avoid is developer error, the best way to do this is to throw an error that makes sense something like "prompt-sync should not be initialized with a string"

If you want to make a pr that throws an intelligent message, that would make more sense :)

ghost commented 7 years ago

Okay! I will do that. Thanks for explaining it!

ghost commented 7 years ago

This ought to do.

/Users/ozzie/Desktop/neon/test.js:8
throw new Error("prompt-sync should not be initialized with a string");
^

Error: prompt-sync should not be initialized with a string
    at Object.<anonymous> (/Users/ozzie/Desktop/neon/test.js:8:7)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:393:7)
    at startup (bootstrap_node.js:150:9)
    at bootstrap_node.js:508:3

Image upload was refusing to cooperate.

ghost commented 7 years ago

Tested,


    try {
        var autocomplete = (config.autocomplete =
            config.autocomplete ||
            function() {
                return [];
            });
    } catch (e) {
        throw new Error("prompt-sync should not be initialized with a string");
    }

now throws:

Error: prompt-sync should not be initialized with a string
    at create (/Users/ozzie/Desktop/neon/node_modules/prompt-sync/index.js:28:9)
    at Object.<anonymous> (/Users/ozzie/Desktop/neon/test.js:2:9)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:393:7)
    at startup (bootstrap_node.js:150:9)
davidmarkclements commented 7 years ago

ah well there's a much easier and obvious way -

check the config parameter with typeof - if it's a string, then throw.

you can do that right at the top of the function, and the intent is clear

also if that piece of code you wrapped threw for some other reason, the error would be misleading

ghost commented 7 years ago

I am an idiot... Thank you. Will fix it.

ghost commented 7 years ago

You are correct, no idea why I didn't think of that earlier. Any way, shall I make another PR for it?

davidmarkclements commented 7 years ago

sure let's close this one off :)