-
-
Notifications
You must be signed in to change notification settings - Fork 282
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.
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
Deno support #316
base: master
Are you sure you want to change the base?
Deno support #316
Conversation
@@ -0,0 +1 @@ | |||
nodejs 14.12.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is generated by asdf to make sure the version of Node is changed automatically when cd
ing into the folder.
I can remove it (committed by mistake).
@@ -35,7 +35,7 @@ function blocks(filepath) { | |||
} | |||
|
|||
const wrap = str => { | |||
return `(function (exports, console, require, module, __filename, __dirname) { ${str}\n});` | |||
return `(function (exports, console, require, module, __filename, __dirname) { ${str}\n});`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were generated by running: eslint fix
with the eslint configuration from the repository.
I hope it's OK.
* enquirer.use(plugin); | ||
* ``` | ||
* @name use() | ||
* @param {Function} `plugin` Plugin function that takes an instance of Enquirer. | ||
* @return {Object} Returns the Enquirer instance. | ||
* @api public | ||
*/ | ||
|
||
use(plugin) { | ||
plugin.call(this, this); | ||
return this; | ||
} | ||
|
||
set Prompt(value) { | ||
this._Prompt = value; | ||
} | ||
get Prompt() { | ||
return this._Prompt || this.constructor.Prompt; | ||
} | ||
|
||
get prompts() { | ||
return this.constructor.prompts; | ||
} | ||
|
||
static set Prompt(value) { | ||
this._Prompt = value; | ||
} | ||
static get Prompt() { | ||
return this._Prompt || require('./lib/prompt'); | ||
} | ||
|
||
static get prompts() { | ||
return require('./lib/prompts'); | ||
} | ||
|
||
static get types() { | ||
return require('./lib/types'); | ||
} | ||
|
||
/** | ||
* Prompt function that takes a "question" object or array of question objects, | ||
* and returns an object with responses from the user. | ||
* | ||
* ```js | ||
* const { prompt } = require('enquirer'); | ||
* const response = await prompt({ | ||
* type: 'input', | ||
* name: 'username', | ||
* message: 'What is your username?' | ||
* }); | ||
* console.log(response); | ||
* ``` | ||
* @name Enquirer#prompt | ||
* @param {Array|Object} `questions` Options objects for one or more prompts to run. | ||
* @return {Promise} Promise that returns an "answers" object with the user's responses. | ||
* @api public | ||
*/ | ||
|
||
static get prompt() { | ||
const fn = (questions, ...rest) => { | ||
let enquirer = new this(...rest); | ||
let emit = enquirer.emit.bind(enquirer); | ||
enquirer.emit = (...args) => { | ||
fn.emit(...args); | ||
return emit(...args); | ||
}; | ||
return enquirer.prompt(questions); | ||
}; | ||
utils.mixinEmitter(fn, new Events()); | ||
return fn; | ||
} | ||
} | ||
|
||
utils.mixinEmitter(Enquirer, new Events()); | ||
const prompts = Enquirer.prompts; | ||
|
||
for (let name of Object.keys(prompts)) { | ||
let key = name.toLowerCase(); | ||
|
||
let run = options => new prompts[name](options).run(); | ||
Enquirer.prompt[key] = run; | ||
Enquirer[key] = run; | ||
|
||
if (!Enquirer[name]) { | ||
Reflect.defineProperty(Enquirer, name, { get: () => prompts[name] }); | ||
} | ||
} | ||
|
||
const exp = name => { | ||
utils.defineExport(Enquirer, name, () => Enquirer.types[name]); | ||
}; | ||
|
||
exp('ArrayPrompt'); | ||
exp('AuthPrompt'); | ||
exp('BooleanPrompt'); | ||
exp('NumberPrompt'); | ||
exp('StringPrompt'); | ||
|
||
module.exports = Enquirer; | ||
export default createEnquirerClass(nodeShims); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main change, using the shims in this entry point (NodeJS one).
The entry point for Deno will be implemented last when the rest works, I can't test it until the changes have all been done anyway.
The contents of this files have been moved to ./lib/enquirer/.js
that is now a function that accepts the shims as parameter.
@@ -1,13 +1,14 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary anymore since module code is always strict.
lib/ansi.js
Outdated
@@ -1,13 +1,14 @@ | |||
'use strict'; | |||
import colors from 'ansi-colors'; | |||
import { isPrimitive } from './utils.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The imports must use full names now, with the extension.
export function isTruthy(value, message) { | ||
if (!value) { throw Error(message); } | ||
} | ||
|
||
export function equals(actual, expected, message) { | ||
if (actual !== expected) { throw Error(message); } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better alternative than using a platform-dependent module since it is so small.
@@ -19,22 +19,28 @@ | |||
"lib" | |||
], | |||
"main": "index.js", | |||
"type": "module", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the long term, I think it would be better to change the files extensions to .mjs
which would allow testing compatibility with CommonJS.
I didn't want to do that now so the diffs are reviewable. Maybe it can come in another PR?
"engines": { | ||
"node": ">=8.6" | ||
"node": ">=12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version 12 of NodeJS is the first to support ESM without experimental flags.
The version 10 can still be supported (see work with bundler below).
Lower versions can also be supported but they are not supported anymore (see schedule).
I don't know if I should fill that field with the lowest supported version (8?), the lowest supported version without bundling but allowing experimental flag (10?) or the lowest supported version without experimental flag (12?).
"lint": "eslint .", | ||
"cover": "nyc --reporter=text --reporter=html mocha", | ||
"build": "parcel build index.js --target node", | ||
"watch": "parcel watch index.js --target node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added linting script and bundler.
Should I fix the last linter errors and add the linting as part of the tests?
.then(answers => { | ||
assert.equal(answers.color, 'orange'); | ||
cb(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to create a formatter configuration for IntelliJ that doesn't change anything in the existing code but failed.
I'm not sure if it is because you are using another editor (.idea is in the .gitignore 🤔) with incompatible settings or if there is no strongly imposed formatting.
"gulp-format-md": "^2.0.0", | ||
"inquirer": "^6.2.0", | ||
"mocha": "^5.2.0", | ||
"mocha": "^8.1.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed to use ESM syntax.
Removes need for cb
in asynchronous tests, mocha will wait for the promises to end on its own now.
|
||
enquirer = new Enquirer({ | ||
show: false, | ||
autofill: true | ||
}, { | ||
color: 'orange' | ||
}) | ||
}); | ||
enquirer.register('foo', Foo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one's interesting, I had to move it down (below the Enquirer instantiation) because it seemed that having registration work on all instances of Enquirer was a side-effect rather than a feature.
Please correct me if I'm wrong and I'll update the code to pass this test like it was originally.
}); | ||
|
||
it.skip('should render a list of choices with the correct styles', cb => { | ||
it('should render a list of choices with the correct styles', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Un-skipped a few tests like this one.
They were running fine.
Tell me if I should comment them again.
export function has(a, b, msg) { | ||
if (Array.isArray(a)) { | ||
assert.ok(Array.isArray(b), 'expected an array'); | ||
for (let i = 0; i < b.length; i++) has(a[i], b[i], msg); | ||
return; | ||
} | ||
|
||
if (typeof a === 'string') { | ||
assert.equal(typeof b, 'string', 'expected a string'); | ||
assert.ok(a.includes(b), msg); | ||
return; | ||
} | ||
|
||
for (const key of Object.keys(b)) { | ||
assert.deepEqual(a[key], b[key], msg); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this file to not try to update assert but instead use it to create the method has.
I had issues with NodeJS refusing to update the assert object, it is also a bit cleaner IMO.
- '14' | ||
- '13' | ||
- '12' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to pass the tests because the ESM syntax does not work on NodeJS < 12.
It should be possible to test the bundled version with a lower version of NodeJS, tell me if I should do it.
9382d7a
to
3498a62
Compare
function isPrompt(object) { | ||
if (!object) { return false; } | ||
return object.name === 'Prompt' || isPrompt(object.__proto__); | ||
} | ||
|
||
const name = type.toLowerCase(); | ||
this.prompts[name] = isPrompt(fn) ? fn : fn(this.Prompt, this); | ||
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part's tough. Now that the classes are built on-demand with platform shims, the previous check: fn instanceof Prompt
does not work anymore.
The method here is a hacky way of trying to stay as close to the previous behavior as possible but it's not clean.
I also tried:
this.prompts[name] = fn.__proto__.name ? fn : fn(this.Prompt, this);
Which works but seems as hacky as the previous version and doesn't do exactly the same thing (only checks whether the argument is a constructor, not that is extends Prompt). Given the fact that constructors that didn't extend Prompt already caused issues (because they were called without new
) I think the solution above is almost acceptable.
if (fns.length === 0) { | ||
onOsSignal(osSignals.SIGTERM).then(() => onExit.bind(null, true, 15)); | ||
onOsSignal(osSignals.SIGINT).then(() => onExit.bind(null, true, 2)); | ||
onOsSignal(osSignals.EXIT).then((...args) => onExit(...args)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing to pass the arguments process.once
to onExit
results in the cursor disappearing in the terminal when running the tests which is super odd and which I haven't understood.
The arguments were already passed through before this commit so I guess you're aware of the issue, if you can explain why I'd be very interested.
Even more so because the arguments are: [0]
which means the code gets passed as first argument which is not what onExit
expects Oo. Something looks fishy to me 🤔
so how is this going? |
This PR is kinda abandoned, cf my comment on the related issue :-( |
This PR is an attempt to add Deno support, see issue #315
I strongly suggest to read the commits separately as there is a lot of boilerplate code modifications. Don't be afraid about the huge number of modified lines, most of them are modified because the classes were wrapped in functions, which indents the full content of a lot of files. The files
./lib/prompts/*.js
and./lib/types/*.js
can be reviewed super-fast even if they account for 80% of the changed lines.Notable parts
I added comments on all non-trivial parts of this PR so it's easier to review.
Current state
Next steps
I'm trying to move the use of
events
to the shims but I encounter an issue I'm not sure how to solve.I tried to change all files that export classes (
lib/prompt.js
,lib/prompts/*.js
,lib/types/*.js
) so they export a method that takes the shims as a parameter and returns the class. This is necessary at least forlib/prompt.js
because the class extendsEventEmitter
fromevents
so I cannot just put the shims as constructor parameter.The issue is that doing so breaks the inheritance because the classes being built dynamically, enquirer can't perform an
instanceof
to detect that the modules registered are instances ofPrompt
.I'll try to dig in and see if I can come up with a fix for this.
Other alternatives
I thought that to take full advantage of ES modules, the API could be updated to be more modular and allow tree-shaking.
I thought about trying to change the API to something like:
index.js
returns only one method:prompt(prompt: Prompt<T>, eventEmitter: EventEmitter): T
lib/prompts
The rationale is that it would:
The problem is that it seems like it would mean removing the plugins so I abandoned that trail of thoughts.