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

Allow timeout: false to remove a timeout #271

Merged
merged 2 commits into from Aug 4, 2019
Merged

Allow timeout: false to remove a timeout #271

merged 2 commits into from Aug 4, 2019

Conversation

jnielson94
Copy link
Collaborator

We ran into a jest issue (jestjs/jest#7890) where the new default timeout causes jest to think that the notification isn't finished after 1 second (which it isn't). I couldn't find a way in the current options (as pointed out in 270) to get the old "fire and forget" behavior.

Related to #218, fix for #270. Allows for timeout: false to overwrite the default timeout set to allow for "fire and forgotten" notifications again.

I added a test for this, and all the tests were passing again. Let me know if you have any issues or comments and I can address them :)

@kevinelliott
Copy link

I would like to see this merged.

@ankurkaushal360
Copy link

I would like to see this merged as well. I am not liking using detectOpenHandles to solve the underlying issue.

@weilinzung
Copy link

release yet?

@mikaelbr
Copy link
Owner

mikaelbr commented Mar 8, 2019

Hm. I'm guessing this now needs to be a major release? Or rather, the previous release was breaking by accident.

@jnielson94
Copy link
Collaborator Author

I don't know that this specific thing needs to be a major release. It does look like the previous one was breaking by accident though - not that it's a huge deal.

At this point I don't know that there'd be a lot of benefit in doing a major release without rolling back the default timeout addition, since you'd want this change available to those who don't want a timeout.

I'm not sure what the timeline for a jest change looks like, but it would require a change there in order to restore the old behavior (since they currently just leave out the timeout option as seen here: https://github.com/facebook/jest/blob/master/packages/jest-reporters/src/notify_reporter.ts#L111-L125 ).

@abhishek-1985
Copy link

Any ETA on when this can be merged ? Also, is there anyway we don't have to wait for jest to make the changes but can set timeout to false to not have jest complain ?

@jmsche
Copy link

jmsche commented Mar 11, 2019

@abhishek-1985 Maybe this can help: in my case I fixed the package version (that's required by webpack-notifier) using the resolutions config in the package.json file:

    "resolutions": {
        "webpack-notifier/node-notifier": "5.3.0"
    }

@mikaelbr
Copy link
Owner

Ok. Looking over this again. Would the best way to solve this (in regards to Jest) be:

  1. Roll back previous version as a patch.
  2. Merging this change and have it be a major release

@jnielson94 ?

@jnielson94
Copy link
Collaborator Author

I think that would work out, assuming you're just rolling back the default timeout and including both changes in a major release. I think the rest of the 5.4.0 changes were good, particularly the no-index on terminal-notifier :)

@abhishek-1985
Copy link

@jnielson94 - do you know when will this get merged ?

@jnielson94
Copy link
Collaborator Author

@abhishek-1985 - I don't. That would be something that @mikaelbr would need to do after rolling back the default timeout change in a patch release :)

@jnielson94
Copy link
Collaborator Author

@mikaelbr - Just wanted to ping you again and check the status of this :)

@artola
Copy link

artola commented Apr 29, 2019

@mikaelbr do you have news about this issue? some ETA?

@jnielson94
Copy link
Collaborator Author

@mikaelbr - So far as I can tell this is still something that needs to be addressed. Is there anything I can do to help with that? :)

@mikaelbr
Copy link
Owner

mikaelbr commented Jul 10, 2019

@jnielson94 Hi! Sorry for taking so long with this. There’s been so much back and forth for this timeout setting And long time has passed that I’ve lost all context. If you could verify that the proposed steps from before work and doesn’t cause other errors (as the reason for the timeout changes in the first place). If you could do that I can do the changes. I could also add you as a collaborator if you would be interessted in doing that?

@jnielson94
Copy link
Collaborator Author

@mikaelbr - Based on this comment from the jest maintainer: jestjs/jest#8036 (comment)

It seems like reverting the default timeout with a patch release of 5.4 would fix the jest bug. So far as adding a default timeout, I would make sure to include a way to disable it (which this PR does, but there could be other ways to accomplish them if you don't like this one) and roll v6 so that jest can update to that and disable the timeout (which they can't do on old versions of jest). Does that make sense?

I could be a collaborator if you need the help?

@mikaelbr mikaelbr merged commit b9946dc into mikaelbr:master Aug 4, 2019
@mikaelbr
Copy link
Owner

mikaelbr commented Aug 4, 2019

All right. I've finally reverted old changes, released 5.4.1, added deprecation notice of 5.4.0, and merged this Pull Request.

I've also added you as a colab, @jnielson94. Would be great to get some help on this. It's been a while since I used this package actively and get less and less connected with the context of various issues.

Would be great if someone who has experienced issues would verify that master is working as expected and then I can do a major release.

@mikaelbr
Copy link
Owner

mikaelbr commented Aug 4, 2019

Oops. Fixed. Sorry about that

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.

None yet

8 participants