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

Simplify namedtuple/object JS objects #310

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Simplify namedtuple/object JS objects #310

merged 1 commit into from
Mar 25, 2022

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Mar 25, 2022

This PR simplifies how EdgeDB's object and namedtuple types are decoded into JS.

Currently we generate custom JS types dynamically for every EdgeDB object and namedtuple. The advantage of that, particularly for namedtuples, is that we can support both tuple[index] and tuple.name notations. The main disadvantage is that some frameworks (cough, nextjs, cough) hardcode JSON serialization and only recognize vanilla JS types, meaning that our custom object/namedtuple types are not supported.

Fix this by changing the code to use vanilla JS objects for both EdgeDB object & namedtuple types.

Backwards-incompatible changes:

  1. namedtuple objects can no longer be index by numbers. I.e. nt[0] expressions are no longer supported. The good news is that we never support proper typing for such expressions.

  2. namedtuple objects no longer have the .length attribute (this is a net improvement though).

  3. Implicit fields for objects are now encoded via symbols. Before, getting a type name was possible with simple obj.__tname__; with this PR it will be obj[Symbol.for('__tname__')].

While there, drop the getSubcodecsNames() method that seems to be unused by edgedb.com and edgedb-studio repos. @jaclarke please confirm.


When landed, we should bump the version to 0.20.0.

@1st1
Copy link
Member Author

1st1 commented Mar 25, 2022

Looking at some test failures I have second thoughts about this...

    Expected: [{Symbol(edgedb.introspect): [Function [edgedb.introspect]], Symbol(nodejs.util.inspect.custom): [Function [nodejs.util.inspect.custom]], Symbol(id): "33a61942abf411eca00987ad4bbbf58c"}, {Symbol(edgedb.introspect): [Function [edgedb.introspect]], Symbol(nodejs.util.inspect.custom): [Function [nodejs.util.inspect.custom]], Symbol(id): "33a7e5ecabf411eca0099b5d9556a442"}, {Symbol(edgedb.introspect): [Function [edgedb.introspect]], Symbol(nodejs.util.inspect.custom): [Function [nodejs.util.inspect.custom]], Symbol(id): "33a85e00abf411eca009ef92353e219b"}]

While using Symbols to add methods/extra info to objects works (Symbols are excluded from JSON.serialize), not using proper types (with proper prototypes) makes dumps like this unnecessarily messy.

Maybe we're better off keeping our code as is and just fix nextjs to serialize things correctly?


The other way to fix this would be:

  1. Always hide implicit id and __tid__ fields (skip them, when decoding data, essentially).
  2. Keep __tname__ only when it's explicitly requested with a higher-level client setting.
  3. Change edgedb studio's inspector to use the codecs tree to render objects and drop the introspectMethod symbol. Also drop the custom inspect function (which will no longer be needed).

@jaclarke thoughts re (3)?

@colinhacks
Copy link
Contributor

colinhacks commented Mar 25, 2022

The root of the issue is that these implicit properties and internal methods are now enumerable and throwing off our expect(...).toEqual(...) checks. I made a PR to your PR that makes all non-enumerable with defineProperty and fixes the tests: #312

I also dropped the Symbol.for() approach for a couple reasons:

  1. It seems like overkill - this is exactly the sort of thing property enumerability is for
  2. The ids that are returned by inserts and shapeless queries are marked as implicit in the protocol, but users (and the query builder) expect to get back a plain {id: string}. This was breaking a handful of tests.

@jaclarke
Copy link
Member

  1. Change edgedb studio's inspector to use the codecs tree to render objects and drop the introspectMethod symbol. Also drop the custom inspect function (which will no longer be needed).

The inspector already works by walking the codecs tree, and I think the only place where it currently needs to use introspect is to get implicit fields on object types, since the object codec doesn't currently keep the flags. (There are a few other places the inspector currently does use introspect, but doesn't really need to, as the information is also in the codec).

@1st1
Copy link
Member Author

1st1 commented Mar 25, 2022

The inspector already works by walking the codecs tree, and I think the only place where it currently needs to use introspect is to get implicit fields on object types, since the object codec doesn't currently keep the flags.

OK, I'm killing the object introspection API then. With it gone we no longer need custom inspect.

The root of the issue is that these implicit properties and internal methods are now enumerable and throwing off our expect(...).toEqual(...) checks. I made a PR to your PR that makes all non-enumerable with defineProperty and fixes the tests: #312

Colin, I've just pushed an even more radical approach:

  1. Drop implicitly requested object IDs (like we do in Go). They're non needed with strict typing and with an absence of operator overloading in JS anyways.

  2. Drop the introspection API and drop the custom inspect.

  3. With 'id' gone, the only implicit name can be __tname__ -- but that's highly special and can only be enabled with undocumented private API (used in EdgeDB Studio only).

With no implicit fields we don't need to use defineProperty.

@1st1
Copy link
Member Author

1st1 commented Mar 25, 2022

Also...

Screen Shot 2022-03-25 at 10 48 51 AM

... must be a good thing

@1st1
Copy link
Member Author

1st1 commented Mar 25, 2022

Forced-pushed a lint fix

While there drop the custom inspect, the introspection API (we can
introspect codecs), and kill implicit object IDs.
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

3 participants