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

make modifier property non-enumerable #21

Closed
geekflyer opened this issue May 4, 2020 · 6 comments
Closed

make modifier property non-enumerable #21

geekflyer opened this issue May 4, 2020 · 6 comments

Comments

@geekflyer
Copy link
Contributor

geekflyer commented May 4, 2020

heyho,

we're using typebox to generate json-schemas and some of them we pass to openapi generator. Some json-schema tools, like openapi generator don't like the fact that the typebox schemas contain the modifier property which isn't actually part of the json-schema spec.
So we wrote some "post-processing" step to remove these modifier properties before passing them to openapi, which "kinda" works but is a bit ugly.

Looking at typebox' code I understand why you need these properties, however I was wondering if you could make these non-enumerable or use a symbol as property name instead? Symbols aren't enumerable via normal means and also get dropped when JSON.stringify()-ing the schema.
This mean that systems like openapi generator wouldn't actually "see" these non-spec compliant properties.

Is that something that typebox could move to?

@sinclairzx81
Copy link
Owner

sinclairzx81 commented May 4, 2020

Hey, that's a great idea :)

I have just pushed an update into the feature/symbol-modifier branch that takes the Symbol approach you suggested. You're right that using a Symbol omits the property from the resulting JSON.stringify which is certainly desirable (leaving a clean standard schema when serialised).

Ill leave this in the branch for you to have a quick review of the updates. If you're happy there (or have amendments you would like to see) happy to review those, otherwise, can publish these changes through later today.

Thanks for the suggestion !

@geekflyer
Copy link
Contributor Author

geekflyer commented May 4, 2020

Heyho, thanks for the quick response. I looked at your branch and I guess this could work but what I actually meant is not to use a symbol for the modifier value, but a symbol for the modifier property key.

So for example instead of:

  /** Modifies the inner type T into an optional T. */
  public static Optional<T extends TSchema | TUnion | TIntersect>(item: T): TOptional<T> {
    return { ...item, modifier: 'optional' }
  }

you could:

const ModifierSymbol = Symbol('TypeBoxModifier');

  /** Modifies the inner type T into an optional T. */
  public static Optional<T extends TSchema | TUnion | TIntersect>(item: T): TOptional<T> {
    return { ...item, [ModifierSymbol]: 'optional' }
  }

This would hide the property also from enumeration on the plain JavaScript object, not only from the serialized JSON.

If you run into problems with this approach (I vaguely remember TypeScript has in some edge-cases problems with typing symbol property names, but not quite sure if that's still the case), you could also just define non enumerable property, such as:

const ModifierKey = '__TypeBoxModifier';

  /** Modifies the inner type T into an optional T. */
  public static Optional<T extends TSchema | TUnion | TIntersect>(item: T): TOptional<T> {
    const optionalizedSchema = { ...item };
    Object.defineProperty(optionalizedSchema, '__TypeBoxModifier', {
      enumerable: false,
      value: 'optional'
    }
    return optionalizedSchema; 
  }

In both cases it's probably also a good idea to export the ModifierSymbol or ModifierKey so that users of typebox can access the modifier if they really want to (may be useful for some custom stuff etc.).

@sinclairzx81
Copy link
Owner

Hey, thanks for the follow up.

I checked in a an extra branch feature/symbol-modifier-key that uses the Symbol as a Key as per suggestion. I do kinda like that the modifier property remains enumerable in some capacity (I feel this is probably less surprising for the end user), but given the nature of the thing, I'm open to it being a non-enumerable key as well.

If you feel the modifier is best expressed as a symbol-key, can roll that branch through instead. As before, the implementation is there for quick review, can publish it through later tonight if all looks good.

Cheers
S

@geekflyer
Copy link
Contributor Author

geekflyer commented May 4, 2020

Ok nice. Checked out the symbol as key branch it looks good to me! Just had a minor comment on the symbol variable name.

Overall I kinda prefer the symbol as key pattern because that's what I think is relatively common - I have seen this particular pattern of using symbols in a couple of other libraries and some of the intros to symbols mention that as well.
The fact that keys which have a symbol value (instead of symbol key) get elided in JSON.stringify() was actually kinda surprising to me, so personally that's another reason I prefer the subjectively lesser "magic" of the symbol as key solution.

Thanks again for the quick response!
Cheers Chris

@sinclairzx81
Copy link
Owner

Hey, alright cool. Symbol as a key it is :) Will also rename to modifierSymbol as per suggestion and push to npm later this evening.

It's a nice update this one. Thanks again for the suggestion!

Cheers
S

@sinclairzx81
Copy link
Owner

Updates published on @sinclair/typebox@0.9.14 :)

Cheers !

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

No branches or pull requests

2 participants