Skip to content
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

feat: add json2md.async method for async converters #32

Merged
merged 2 commits into from
Jan 21, 2018

Conversation

adoyle-h
Copy link
Contributor

@adoyle-h adoyle-h commented Jan 7, 2018

  • json2md.async will return a Promise for asynchronous handle

- json2md.async will return a Promise for asynchronous handle
if (typeof func === "function") {
return Promise.resolve()
.then(() => func(_type ? data : data[type], json2md))
.then((result) => indento(result, 1, prefix) + "\n")
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry if I'm missing anything, but isn't this still sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The func can be either asynchronous or synchronous function.

For examples:

json2md.converters.md = async (filepath) => {
    return await Promise.fromCallback((cb) => FS.readFile(filepath, {encoding: 'utf8'}, cb));
};

json2md.converters.asyncResult = () => Promise.resolve('async');

@IonicaBizau
Copy link
Owner

I love the idea. Thanks for this!

However, I'm not sure what async would mean in this case. Just because sync functions are wrapped around in promises, doesn't make them really async.

@adoyle-h
Copy link
Contributor Author

adoyle-h commented Jan 7, 2018

Yes, sync functions could be wrapped around in promises. json2md.async return a Promise, so it is different from json2md function signature. The new entrypoint just for package version compatibility.

@adoyle-h
Copy link
Contributor Author

adoyle-h commented Jan 8, 2018

Any problems? @IonicaBizau

@IonicaBizau
Copy link
Owner

Um, I see your point, but in what context would any of the converter functions be async? They are supposed to be small, sync functions.

@adoyle-h
Copy link
Contributor Author

I want to read content from file, database, CDN or other services, not only transform content in converter. A part of content may be fetched from the outside of process rather than static variables in codes. For this scenario, the data passed to json2md is not pure static data, but a condition for fetching and transforming data. json2md.async([{type: data}, {type: condition}])
Although NodeJS provides some sync functions such as fs.readdirSync and fs.readFileSync, I think any interactions with the disk and network should be async.
It will be more flexible for user if converter could support both async and sync functions.

@IonicaBizau
Copy link
Owner

Ah, I understand now. 🚀

Can you add some tests before merging?

@adoyle-h
Copy link
Contributor Author

OK. It may take a little time.

@adoyle-h
Copy link
Contributor Author

@IonicaBizau done.

@IonicaBizau IonicaBizau mentioned this pull request Jan 21, 2018
@IonicaBizau IonicaBizau merged commit 01c5929 into IonicaBizau:master Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants