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

Add catch-or-return #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

voxpelli
Copy link
Member

@voxpelli voxpelli commented Jul 6, 2019

@LinusU
Copy link
Member

LinusU commented May 28, 2020

Ecosystem breakage report:

  • fs-extra
    • lib/copy/copy.js:54:3
  • simple-peer
    • index.js:701:7
  • standard-engine
    • bin/cmd.js:95:5
  • webtorrent
    • lib/file.js:109:5
    • lib/file.js:118:5
  • har-validator
    • test/promise.js:23:3
  • read-closest-package
    • index.js:28:5
  • fs-writefile-promise
    • test/index.js:17:3
  • aerospike
    • test/batch_exists.js:69:5
    • test/batch_get.js:69:5
    • test/batch_select.js:69:5
    • test/command_queue.js:29:7
    • test/command_queue.js:33:11
    • test/command_queue.js:46:7
    • test/command_queue.js:50:11
    • test/command_queue.js:63:7
    • test/get.js:116:5
  • ban-sensitive-files
    • bin/ban.js:40:1
  • addressbar
    • tests/index.js:16:3
    • tests/index.js:19:3
    • tests/index.js:26:3
    • tests/index.js:29:3
    • tests/index.js:36:3
    • tests/index.js:39:3
    • tests/index.js:47:3
    • tests/index.js:52:3
    • tests/index.js:56:3
    • tests/index.js:65:3
    • tests/index.js:70:3
    • tests/index.js:75:3
    • tests/index.js:79:3
    • tests/index.js:87:3
    • tests/index.js:92:3
    • tests/index.js:96:3
    • tests/index.js:105:3
    • tests/index.js:110:3
    • tests/index.js:115:3
    • tests/index.js:119:3
    • tests/index.js:126:3
    • tests/index.js:131:3
    • tests/index.js:136:3
    • tests/index.js:140:3
    • tests/index.js:149:3
    • tests/index.js:154:3
    • tests/index.js:160:3
    • tests/index.js:164:3
    • tests/index.js:172:3
    • tests/index.js:175:3
    • tests/index.js:180:3
    • tests/index.js:183:3
    • tests/index.js:188:3
    • tests/index.js:191:3
    • tests/index.js:196:3
    • tests/index.js:199:3
    • tests/index.js:203:3
    • tests/index.js:211:3
    • tests/index.js:214:3
    • tests/index.js:219:3
    • tests/index.js:222:3
    • tests/index.js:227:3
    • tests/index.js:230:3
    • tests/index.js:235:3
    • tests/index.js:238:3
    • tests/index.js:243:3
    • tests/index.js:246:3
    • tests/index.js:251:3
    • tests/index.js:254:3
    • tests/index.js:259:3
    • tests/index.js:262:3
    • tests/index.js:267:3
    • tests/index.js:270:3
    • tests/index.js:274:3
    • tests/index.js:282:3
    • tests/index.js:287:3
    • tests/index.js:292:3
    • tests/index.js:295:3
    • tests/index.js:300:3
    • tests/index.js:305:3
    • tests/index.js:310:3
    • tests/index.js:313:3
    • tests/index.js:317:3
    • tests/index.js:324:3
    • tests/index.js:327:3
    • tests/index.js:330:3
    • tests/index.js:333:3
    • tests/index.js:336:3
    • tests/index.js:339:3
    • tests/index.js:342:3
    • tests/index.js:347:3
    • tests/index.js:350:3
    • tests/index.js:354:3
  • tape-promise
    • index.babel.js:68:36
    • tests/async.test.js:15:3
    • tests/async.test.js:18:7
  • instant.io
    • client/index.js:267:11
  • studynotes.org
    • liveupdater/index.js:261:5
    • tools/transform-css.js:13:1
  • bitmidi.com
    • src/api/midi.js:143:5
  • webtorrent-desktop
    • bin/package.js:530:5
    • bin/package.js:564:5
    • src/renderer/webtorrent.js:360:3
    • test/index.js:10:3
    • test/test-add-torrent.js:11:3
    • test/test-add-torrent.js:60:3
    • test/test-audio.js:9:3
    • test/test-torrent-list.js:13:3
    • test/test-torrent-list.js:32:3
    • test/test-torrent-list.js:62:3
    • test/test-video.js:9:3
  • sinon-as-promised
    • examples/angular/test.js:22:5
    • test.js:26:5
    • test.js:28:7
    • test.js:37:5
  • testdouble
    • test/safe/callback.test.js:128:9
    • test/safe/when.test.js:163:9
    • test/safe/when.test.js:171:9
    • test/safe/when.test.js:172:11
    • test/safe/when.test.js:182:9
    • test/safe/when.test.js:189:9
    • test/safe/when.test.js:190:11
    • test/safe/when.test.js:220:9
    • test/safe/when.test.js:227:9
  • fastify
    • lib/contentTypeParser.js:116:7
    • lib/contentTypeParser.js:200:7
    • lib/hooks.js:138:9
    • lib/hooks.js:158:7
    • lib/hooks.js:198:7
    • lib/route.js:411:7
    • lib/wrapThenable.js:16:3
    • test/content-length.test.js:154:3
    • test/decorator.test.js:707:3
    • test/inject.test.js:347:3
    • test/internals/reply.test.js:1360:5
    • test/internals/reply.test.js:1374:5
    • test/listen.test.js:98:3
    • test/listen.test.js:113:3
    • test/listen.test.js:210:3
    • test/listen.test.js:220:3
    • test/listen.test.js:230:3
    • test/listen.test.js:240:3
    • test/listen.test.js:250:3
    • test/listen.test.js:260:3
    • test/listen.test.js:349:3

I don't have much time now but from a quick glance on some of them it seems like they are potentially false positives, which is not great 🤔

I think we need to do some more digging here...

@voxpelli
Copy link
Member Author

voxpelli commented Jun 2, 2020

Right, yeah for sinon-as-promised (which has been superseded by sinon 2.x) it's a bluebird + tape thing set up to handle the unhandled rejections:

https://github.com/bendrucker/sinon-as-promised/blob/c0fa8e411c450c4399be911a2da6f9350e5e9ef4/test.js#L9-L13

For fastify there seems to be a combination of using a .then(handleResolution, catchRejection) and specifics in test setup.

Maybe we would need to get an option to treat such calls as ok @LinusU?

@voxpelli
Copy link
Member Author

I would be 👍 on shipping this as a warn in 17.0.0, thoughts @feross @LinusU @divlo and others?

@LinusU
Copy link
Member

LinusU commented Oct 15, 2021

I'm not too sure about this, from the discussion in jprichardson/node-fs-extra#622 it seems like there isn't really a straight forward way to work around this.

I think that we should have some suggestions for the affected packages on what they should change to? 🤔

@voxpelli
Copy link
Member Author

@LinusU Good feedback 👍 I'm leaning more towards this after having slept on it as well. Lets try and target this for 18.0.0 instead then (I've set up some milestones to draft it up)

@rostislav-simonik rostislav-simonik added the scope:rule Concerning linting rules label Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:rule Concerning linting rules
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Enable rule "promise/catch-or-return"?
4 participants