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

hide error symbol from being printed #168

Merged
merged 4 commits into from
Oct 17, 2024

Conversation

CatchMe2
Copy link
Collaborator

Changes

This PR prevents the error symbol from being printed.
console.log output of the previous implementation:
image

console.log output of the new implementation
image

Checklist

  • Apply one of following labels; major, minor, patch or skip-release
  • I've updated the documentation, or no changes were necessary
  • I've updated the tests, or no changes were necessary

@drdaemos
Copy link
Collaborator

Is there is a specific reason to this change?
e.g. why don’t we want to have this symbol printed?

@@ -1,4 +1,4 @@
import { isError } from '../utils/typeUtils'
import { isNativeError } from 'node:util/types'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? this method was deprecated AFAIR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not depreciated: https://nodejs.org/api/util.html#utiltypesisnativeerrorvalue
I think that it's better to use std lib solutions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then it makes sense

@CatchMe2
Copy link
Collaborator Author

Is there is a specific reason to this change? e.g. why don’t we want to have this symbol printed?

This symbol was meant to be internal, used for instance check only. It doesn't provide any valuable info.

@CatchMe2
Copy link
Collaborator Author

After a discussion, we decided to leave printing it as before.

@CatchMe2 CatchMe2 force-pushed the hide-error-symbol-from-being-printed branch from 49da2a9 to f50a6c0 Compare October 16, 2024 09:14
@CarlosGamero
Copy link
Contributor

Not sure if related, but I am getting the following on my PR:

Property '[internalErrorSymbol]' is missing in type 'import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").InternalError<import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").ErrorDetails>' but required in type 'import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").InternalError<import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").ErrorDetails>'.

https://github.com/lokalise/autopilot/actions/runs/11363730214/job/31608342987?pr=4496

@CatchMe2
Copy link
Collaborator Author

Not sure if related, but I am getting the following on my PR:

Property '[internalErrorSymbol]' is missing in type 'import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").InternalError<import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").ErrorDetails>' but required in type 'import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").InternalError<import("/Users/[email protected]/development/Node/autopilot/node_modules/@lokalise/node-core/dist/index").ErrorDetails>'.

https://github.com/lokalise/autopilot/actions/runs/11363730214/job/31608342987?pr=4496

Using

Object.defineProperty(InternalError.prototype, INTERNAL_ERROR_SYMBOL, {
  value: true,
})

Should resolve this problem as it would make the SYMBOL property invisible to type checking as well.

@CatchMe2 CatchMe2 force-pushed the hide-error-symbol-from-being-printed branch from f50a6c0 to 0e035b5 Compare October 16, 2024 13:31
@CatchMe2 CatchMe2 force-pushed the hide-error-symbol-from-being-printed branch from 0e035b5 to 49957db Compare October 16, 2024 13:34
@CatchMe2 CatchMe2 merged commit f857771 into main Oct 17, 2024
5 checks passed
@CatchMe2 CatchMe2 deleted the hide-error-symbol-from-being-printed branch October 17, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants