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

Integrating JSON serialization and deserialization #2

Merged
merged 3 commits into from
May 9, 2022
Merged

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented May 7, 2022

Description

Previously in #1, I had tried to add generic toJSON and fromJSON methods into AbstractError, however I couldn't figure out the types and I realised that it was an application concern. Now that @emmacasolin has prototyped how it works in PK, I can bring this functionality back into js-errors.

The proper insight is to realise that toJSON is used by JSON.stringify without bothering with the replacer, but the replacer would still be used by errors outside the AbstractError class hierarchy such as standard JS errors. And to not bother with the cause property, and just leave it as is, since JSON.stringify will traverse into it anyway.

Then on the deserialisation side, we wanted to apply a runtime deserialisation associated with each class, while also inheriting the default deserialisation logic if nothing changed. This was done with a static method fromJSON. However the static methods are constrained by function-subtyping, but by making use of the any type and the Class utility type we are able to make fromJSON very generic, such that child classes can override it. This is also used by a reviver that still needs to deal with errors outside the AbstractError hierarchy.

I've now created a minimal prototype serialisation/deserialisation logic synthesised from the discussion MatrixAI/Polykey#304 (comment).

The reviver side is much more complicated than the replacer side. Obviously because you have to deal with unknown input.

As written in the comments, there are ultimately 3 choices when dealing with unknown error data that we want to deserialise.

  1. throw an "parse" exception
  2. return the value as it is
  3. return as special "unknown" exception that contains the unknown value as data
  • Choice 1. results in strict deserialisation procedure (no forwards-compatibility)
  • Choice 2. results in ambiguous parsed result
  • Choice 3. is the best option as it ensures a typed-result and debuggability of ambiguous data

So the example goes with choice 3, and I think for production applications that needs to deal with backwards and forwards compatibility, it will be important to have that unknown error with associated data. However you can see in the tests that this is only ever done at the root of the deserialised error, all other data is returned as-is if it could not be decoded properly.

Issues Fixed

Tasks

  • 1. Add toJSON to AbstractError
  • 2. Add static fromJSON to AbstractError
  • 3. Add tests demonstrating error serialisation and deserialisation
  • 4. Update to node v16 and ensure that the tests show that the cause property available on standard JS errors in the tests for serialisation and deserialisation - cause is actually ES2022, so it is not done https://node.green/

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

Note that in node v16, normal errors can also have a cause property, so the reviver and replacer should handle that too.

@CMCDragonkai
Copy link
Member Author

I'll update to node v16 here too.

@CMCDragonkai
Copy link
Member Author

@emmacasolin if you are happy with reviewing the the code, then I'll update to v16 as a separate commit, and then push a release.

Then back to MatrixAI/Polykey#304 and we can update to this latest package, and make use of the fromJSON and toJSON, and just adapt the replacer and reviver accordingly.

We will then still need to do the error filtering in PK when going from agent to agent, but that's a separate concern. And I was thinking that the stack is always filtered even when going from agent to client. I'm also wondering if error filtering is related between the server-side logging of the errors vs the filtering of error data being sent over. It's like they are separate concerns as well, and maybe it's really that we need to distinguish client errors from server errors.

tests/index.test.ts Outdated Show resolved Hide resolved
@CMCDragonkai CMCDragonkai changed the title WIP: Integration JSON serialisation and deserialiastion WIP: Integration JSON serialization and deserialization May 8, 2022
@CMCDragonkai CMCDragonkai changed the title WIP: Integration JSON serialization and deserialization WIP: Integrating JSON serialization and deserialization May 8, 2022
@CMCDragonkai
Copy link
Member Author

All done. BTW @emmacasolin I had to change to env: { "es2021": true } in .eslintrc to actually enable ES2021 features. I'm adding that the TS demo lib too, but you should do this in PK when transitioning to nodev16.

@CMCDragonkai CMCDragonkai changed the title WIP: Integrating JSON serialization and deserialization Integrating JSON serialization and deserialization May 9, 2022
@CMCDragonkai CMCDragonkai merged commit f9ab31c into master May 9, 2022
@CMCDragonkai CMCDragonkai deleted the json branch May 9, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants