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

Do not throw on custom stack traces #1491

Merged
merged 4 commits into from Nov 2, 2020
Merged

Conversation

tabarra
Copy link
Contributor

@tabarra tabarra commented Oct 7, 2020

Changed error.stack type checking (resolves #1490).
Same tests apply for normal node. Hard to run tests inside fxserver.

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

@szmarczak
Copy link
Collaborator

This needs a test.

@tabarra
Copy link
Contributor Author

tabarra commented Oct 7, 2020

Can you provide an example of a test that should be done?
The existing ones are still valid, and will check for the error behavior in normal Node.
For FXServer, it's probably impossible to run the same automated tests...

None of these commands will work inside fxserver:

"test": "xo && npm run build && nyc --reporter=html --reporter=text ava",
"build": "del-cli dist && tsc",

As I see, this doesn't affect Standard Node at all, specially with the current test coverage.
But with this small change it will also work for FiveM's FXServer - which is impossible to test.

@szmarczak
Copy link
Collaborator

Can you provide an example of a test that should be done?

Please see the source code, there already hundreds of them. Here you just need to create an error and override the .stack property and pass it to RequestError and make sure the error creation is successful.

@tabarra
Copy link
Contributor Author

tabarra commented Oct 7, 2020

I believe that all that could be tested well is already being tested.
This issue happens inside the RequestError constructor, and doesn't depend on any input like an Error object, so passing/altering the error stack won't make any difference.
As I see, the only way for me to write a test for this would be if I monkey-patch Error.captureStackTrace() to return a parsed stack instead of a string.
If you think its better not to pollute the environment, I can remove it.

But... it really that necessary?
We are already testing the working/behavior of RequestError, but this.stack is being treated as a string, even though we are not testing if it is a string or not.

Alternatively I could just do a new PR with a try/catch for the offending lines. Please do let me know.

@szmarczak
Copy link
Collaborator

No, that won't be necessary. I'll do the tests, no problem :)

@tabarra
Copy link
Contributor Author

tabarra commented Oct 10, 2020

Okay, you can edit this PR if you need to.
And let me know if there is anything else I can do.

@NonStopGamer
Copy link

@szmarczak Any ETA for the merge?
I need to update one dependency for my project due to a security vulnerability, but I can't before you merge this PR.

@tabarra
Copy link
Contributor Author

tabarra commented Oct 24, 2020

Guys, any news on that?
This is a very simple PR to fix a simple issue...
The code treats as array something that is not being testes if it's an string.

@szmarczak
Copy link
Collaborator

Sorry, haven't had the time yet.

@Giotino would you be up to this? If not then I'll try to do this tomorrow evening.

@szmarczak szmarczak changed the title Changed error.stack type checking (resolves #1490) Do not throw on custom stack traces Nov 2, 2020
@szmarczak szmarczak merged commit 49c16ee into sindresorhus:master Nov 2, 2020
@tabarra
Copy link
Contributor Author

tabarra commented Nov 2, 2020

Thank you very much :D

@tabarra
Copy link
Contributor Author

tabarra commented Nov 23, 2020

Hey @szmarczak , do you mind publishing this patch?
Last one was a few days prior to this merge.
Thank you.

@synterrr
Copy link

synterrr commented Dec 5, 2020

@szmarczak where are you guys going to release it, my lib it's with a critical error because it doesn't have updates

@szmarczak
Copy link
Collaborator

No ETA yet. Does got@9.6.0 work for you? If not, you can build manually from master.

@tabarra
Copy link
Contributor Author

tabarra commented Dec 10, 2020

It is a dependency of openid-client so I cannot control the version being used.
Because of that, I'm stuck on openid-client v3.15.9 while the latest is v4.2.2, and I really rather not have my authentication library outdated :p

Do you see any other way I could solve this? Or is there any way for you guys to push a new tag for got?
I don't particularly understand why this fix was not yet published. If there is anything I can do to help please let me know.
Sorry for being anxious, but there are currently ~3000 fxserver instances being protected by an outdated library :(

@szmarczak
Copy link
Collaborator

Because of that, I'm stuck on openid-client v3.15.9 while the latest is v4.2.2, and I really rather not have my authentication library outdated :p

Ok, I will now set a separate branch for v10 and make an NPM patch release.

szmarczak pushed a commit that referenced this pull request Dec 10, 2020
@szmarczak
Copy link
Collaborator

Published got@11.8.1. Please do let me know if that works for you.

@szmarczak
Copy link
Collaborator

I don't particularly understand why this fix was not yet published.

Master has got already breaking changes. v12 is in the works therefore v11 development has stopped I'd say.

@tabarra
Copy link
Contributor Author

tabarra commented Dec 18, 2020

It works for me, thank you very very much!

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.

Uncaught TypeError when running from inside FiveM's FXServer.
4 participants