add example with for await #106
Conversation
There was a problem hiding this comment.
(thanks for opening a new PR. moving forward, please do fill out the PR template)
Left some comments on things that need to be modified to accept the PR. Lots of maintainer tasks that shouldn't go into a docs update PR. Please also remove package-lock.json from the commit. That's a file that maintainers should be curating.
I was about to update my last comment on #92 when you opened this, so I both apologize I didn't get that done in time to continue the discussion before you created this, and thank you for taking the time to create another PR. I'd like to get your thoughts:
After looking into for await of/asyncIterator more, I'm not really convinced this is a good example to add at the moment. node.green isn't tracking asyncIterator yet, it's still in proposal, and requires a harmony flag or babel which may throw some users off without being thoroughly documented.
.eslintrc
Outdated
| @@ -13,6 +13,7 @@ | |||
| "prefer-template": "error" | |||
| }, | |||
| "settings": { | |||
| "import/parser": "babel-eslint", | |||
There was a problem hiding this comment.
ah yeah this is a bad change. renaming the file is no good and adding additional config to it for the examples isn't worth it. the project doesn't use babel explicitly, so it's best to just ignore the offending example file in .eslintignore.
There was a problem hiding this comment.
Oh right, I saw an error in my editor, but it's indeed not needed, removed
.travis.yml
Outdated
| - 10 | ||
| - 8 | ||
| - 6 | ||
| - "10" |
There was a problem hiding this comment.
please leave CI up to maintainers and revert these changes :)
There was a problem hiding this comment.
as you want
(using yaml extension with schemas for travis, etc..)
There was a problem hiding this comment.
understood, but that's still a CI commit on a docs PR for an error that's only being displayed on your end :)
| @@ -1,6 +1,6 @@ | |||
| const write = require('csv-write-stream') | |||
| const through = require('through2') | |||
There was a problem hiding this comment.
there's value in showing how to use this with through2 since that's still widely used. renaming the original transform.js to transform-through2.js and then modifying this file would've been a better route.
There was a problem hiding this comment.
I feel like using a builtin module doesn't add complexity, shouldn't we encourage builtins in that case?
There was a problem hiding this comment.
it doesn't, and we should. however, through2 is still getting 4 million downloads a month and is still widely used. removing its usage is removing value for users. that's why I suggested renaming the file in its original state to express that it's a through2 example. that way we keep both.
package.json
Outdated
| "eslint": "^5.4.0", | ||
| "eslint-config-standard": "^11.0.0", | ||
| "eslint-plugin-import": "^2.14.0", | ||
| "eslint-plugin-node": "^7.0.1", | ||
| "eslint-plugin-promise": "^4.0.0", | ||
| "eslint-plugin-standard": "^3.1.0", | ||
| "eslint-plugin-standard": "^4.0.0", |
There was a problem hiding this comment.
please allow maintainers to manage dependency updates and revert this change
|
package-lock.json is already committed, it's a file that change when installing a new package, so it's normal that it changed asyncIterator needs no flag on node10 (LTS) I appreciate that people like you try to maintain packages, but I regret how inefficient the process is Concerning also I don't think this commit will make it, too bad, have a good week, will close some time later |
|
No worries. I'm sorry you found the process to be inefficient, and regrettable on your decision. If you'd like to reopen later, please let us know. |
Added an example using asyncIterator (
for await)