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

Don't freeze signal when freezing Options #2100

Merged
merged 2 commits into from Aug 5, 2022

Conversation

svvac
Copy link
Contributor

@svvac svvac commented Aug 4, 2022

Freezing the abort signal object breaks its functionality under Node.

Fixes #2099

@szmarczak
Copy link
Collaborator

Can you add a test please?

@svvac
Copy link
Contributor Author

svvac commented Aug 4, 2022

What shall I test? That the signal is frozen? That it can be aborted?

@svvac
Copy link
Contributor Author

svvac commented Aug 4, 2022

Added tests :

  • Asserts the signal does not get frozen when calling Options.freeze()
  • Asserts that a signal passed as a default option to Got.extend() can be aborted (detection will depend on the runtime)

@svvac
Copy link
Contributor Author

svvac commented Aug 4, 2022

Re: failure under node 14, would you rather have the freeze test skipped, or the signal object be mocked when AbortController is not defined?

@szmarczak
Copy link
Collaborator

Skip the test like in test/abort.ts

@szmarczak
Copy link
Collaborator

@svvac Please avoid force-pushing unless you need to fix merge conflicts. It really makes code review harder.

test/normalize-arguments.ts Outdated Show resolved Hide resolved
Co-authored-by: Szymon Marczak <36894700+szmarczak@users.noreply.github.com>
@szmarczak szmarczak changed the title Don't freeze signal when freezing Options (#2099) Don't freeze signal when freezing Options Aug 5, 2022
@szmarczak szmarczak merged commit 43b1467 into sindresorhus:main Aug 5, 2022
9 checks passed
@szmarczak
Copy link
Collaborator

Thanks! 🙌🏼

@svvac svvac deleted the dont-freeze-signal branch August 5, 2022 13:39
@sindresorhus
Copy link
Owner

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.

Options freezing breaks abort signal
3 participants