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

feature: add optional flatMode flag to flatten surrealdb responses to plain js objects #244

Closed
wants to merge 1 commit into from

Conversation

oskar-gmerek
Copy link

@oskar-gmerek oskar-gmerek commented Apr 24, 2024

What is the motivation?

Switching from JSON to CBOR and the subsequent introduction of classes such as RecordId significantly lowers the DX in its current form, for example, when working with frameworks like SvelteKit. SvelteKit only supports data transmission between the server and the client in JSON format, and attempting to send the current response from SurrealDB will result in an error Error: Data returned from 'load' while rendering / is not serializable: Cannot stringify arbitrary non-POJOs.

The toJSON() method available in the RecordId class solves this problem, but it does not improve DX because before any data can be sent, it must first be destructured and the method used in all occurrences of RecordId.

What does this change do?

This change introduces an optional flatMode flag in the constructor of the Surreal class. When flatMode: true, all responses from SurrealDB that may contain RecordId are recursively flattened into a JavaScript object format via the flatten() function.

Creating new instances:
Regular: new Surreal()
flatMode: new Surreal({ flatMode: true })

Example Outputs:
Regular:

{
   id: RecordId {
      tb: "person", 
      id: 1
   },
   firstname: "John",
   lastname: "Doe",
   age: 30,
},

flatMode:

{
   id: {
      tb: "person", 
      id: 1
   },
   firstname: "John",
   lastname: "Doe",
   age: 30,
},

What is your testing strategy?

I added tests based on existing ones, each test includes the suffix - flatMode. I also compared the performance between the query and query - flatMode tests, which show a performance difference of less than 1ms.

Is this related to any issues?

#247

Related to, but this is partial or alternative solution for #241 #242

Also I saw related messages on Discord (but this is partial or alternative solution):
Zrzut ekranu 2024-04-24 o 11 44 02

Zrzut ekranu 2024-04-24 o 11 31 15 Zrzut ekranu 2024-04-28 o 19 33 15

To do

  • add support for live queries
  • add tests for live queries

Have you read the Contributing Guidelines?

@dodanex
Copy link

dodanex commented Apr 28, 2024

@oskar-gmerek agree, the ID should be in the root of the object, not nested!

@abrisene
Copy link

+1 - I have abstraction around instantiating a surreal client in 0.11, but the changes to the ids makes migration more complicated than I'd like.

It would be nice to have this option, which would make it easier to migrate more gradually.

Ideally I'd want to have the option to flatten on the query methods themselves since cbor makes it much easier to use complex ids, but changing the signature of each of the query methods seems like a bad idea.

@dodanex
Copy link

dodanex commented May 1, 2024

My opinion is that flat mode should be the default mode:

Creating new instances:
Regular with flat mode: new Surreal()
With flatMode off: new Surreal({ flatMode: false })

What do you think @kearfy , @oskar-gmerek ?

@kearfy
Copy link
Member

kearfy commented May 6, 2024

Hey @oskar-gmerek! Thanks for working on this PR. I don't think we should support global config options like this, we could however improve the toJSON methods in such a way that you can simply do

await db.select('table-name').then(r => JSON.stringify(r))

And get roughly the same result back as the previous versions of the library.
It sounds quite trivial to construct a string RecordID outside the rust library, but it's actually not. Take tb: table:name and id something-else as an example:

"table:name:something-else"

You now end up with an invalid record id, as the two ident parts are not escaped. Now let's say that you not send this to SurrealDB, but you would have RecordId.fromString("table:name:something-else"), it would also result in something else where tb is table and the id part is name:something-else.

What I'm boiling down to is that, yes, we can do this, however it would add the cost of using regex which at scale can massively degrade performance. Ideally implementors would adjust to the new library and take advantage of it's understanding of datatypes native to SurrealQL.

I'll close this PR, but I'll see what I can do to add in a JSON compatibility mode. It will however carry a price tag on performance, and you won't be able to interchange some of this values back to the server without manually converting them back to these new representations of native SurrealQL datatypes, which includes RecordId, UUID, Datetime, Duration, Decimal, BigInt, Table and Geometries.

@kearfy kearfy closed this May 6, 2024
@kearfy kearfy mentioned this pull request May 6, 2024
1 task
@kearfy
Copy link
Member

kearfy commented May 6, 2024

Hey all! As you can see, I just merged a PR which allows you to get a JSON-like representation of the responses the way that SurrealDB would also display JSON-like responses.

Here's an example of what you'll be able to do in the next beta:

import { StringRecordId, jsonify } from 'surrealdb.js';

// Use the selection, creation and altering methods with string record ids
const res = await db.select(new StringRecordId("person:john"));

// You will get back a response with native values
// However will `jsonify()` or `JSON.stringify()` you will get back a JSON representation of the result, the way that SurrealDB would also display it
const jsonlike = jsonify(res);
const jsonstring = JSON.stringify(res);

// You can also easily do this in one go
await db
    .select(new StringRecordId("person:john"))
    .then(jsonify);

We added the jsonify() method just in #259. It does a basic JSON stringify and parse. We will investigate if something custom can be more performant and will catch all edge cases. Currently the added benefit of the jsonify() method is that it will transform the types of the passed value to represent the actual JSON output.

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

4 participants