Add boolean to detect object type #40

jsumners · 2019-07-31T13:36:56Z

The issue I was having in #39 was due to object composition and cloning. I have a wrapper around Fastify that reads a list of routes from config/routes.js and then the corresponding handlers from lib/handlers/. For example:

config/routes.js:

module.exports = [{
  path: '/foo',
  method: 'GET',
  handler: 'foo'
}]

lib/handlers/foo.js:

module.exports = {
  schema: {
    query: { fluent.object().prop('foo', fluent.string()) }
  },
  handler(req, res) {
    res.send(req.query)
  }
}

So these two files are read in and the objects are merged to build up the full route definition before being passed to fastify.route. If any of the schemas are fluent-schema instances then the Symbol.for('fluent-schema-object') will be lost during this process because symbols do not get copied in any current object merging method (by design). But if we don't retain the ability to determine if the schemas are instances of fluent-schema then we can't work with these schemas during the schema compilation process. Thus, this PR adds an isFluentSchema boolean to use instead of the symbol.

mcollina · 2019-07-31T16:49:40Z

Would you mind adding that to the docs?

jsumners · 2019-07-31T17:08:32Z

I don't see anything in docs/API.md for the symbol so I didn't add it for this. The only mention of this type of stuff is in the README.md.

mcollina · 2019-07-31T17:46:52Z

adding this to the readme would be good as well. Il giorno mer 31 lug 2019 alle ore 19:08 James Sumners < notifications@github.com> ha scritto:

…

I don't see anything in docs/API.md for the symbol so I didn't add it for this. The only mention of this type of stuff in in the README.md. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY6UGXXQ6OVX2SGJHELQCHBJBANCNFSM4IIGYR5Q> .

jsumners · 2019-07-31T17:49:35Z

I did add it to the readme.

jsumners · 2019-07-31T19:14:49Z

I have updated API.md with bits for both properties. I think the symbol is extraneous, though, and should be removed.

mcollina

LGTM

delvedor

LGTM

At this point why keep the symbol at all?

mcollina · 2019-08-01T09:46:10Z

backward compat for now, we should drop in the next major ideally. However, it's not that big of a deal to keep Il giorno gio 1 ago 2019 alle ore 11:25 Tomas Della Vedova < notifications@github.com> ha scritto:

…

***@***.**** approved this pull request. LGTM At this point why keep the symbol at all? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY4QISFQRGR637EQKNTQCKTZ7ANCNFSM4IIGYR5Q> .

jsumners · 2019-08-01T10:53:59Z

Merge release and I’ll submit a PR to Fastify.

mcollina · 2019-08-01T13:28:35Z

I'd prefer to wait for @aboutlo opinion.

aboutlo · 2019-08-01T13:55:05Z

Currently in Crete I will have a look over the weekend and get back to you

On Thu, 1 Aug 2019 at 16:28, Matteo Collina ***@***.***> wrote: I'd prefer to wait for @aboutlo <https://github.com/aboutlo> opinion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJN2LBRK2MN6GOTKPGOY2LQCLQIJANCNFSM4IIGYR5Q> .

-- Sent from mobile phone

aboutlo · 2019-08-03T09:12:07Z

hey, @jsumners thank you for opening the PR!

A couple of things:

API.md is a file autogenerated from doc. I would add the explanation just in the README
Let's just put the new method only in the README, but we keep the old one as well under the hood so no breaking changes

So these two files are read in, and the objects are merged to build up the full route definition before being passed to fastify.route

Could you add a test to reproduce your cloning issue? Maybe in the Integration tests

I would expect to have similar issues given I do a lot of deconstructing internally at FluentSchema, but maybe I have been lucky.

I'm worried that Fluent Schema has an internal util method https://github.com/fastify/fluent-schema/blob/5382d9bfce1d630356878289e1abb06a57057882/src/utils.js#L3 that has been changed to use the symbol.

So if we can reproduce the issue I would expect we need to move to the boolean implementation in order to fix it

jsumners · 2019-08-03T12:04:22Z

Good call on the integration test. It seems I was wrong about Object.assign and {...foo} in regard to symbols. They actually do copy such properties over. The actual issue is my usage of https://www.npmjs.com/package/lodash.merge which "... recursively merges own and inherited enumerable string keyed properties of source objects into the destination object."

I see that I can probably rework how I'm cloning my objects in my project (possibly), but I also think the usage of a Symbol here is overkill and just being crafty for the sake of being crafty. So I still think migrating to a plain boolean is best.

aboutlo · 2019-08-03T13:57:49Z

I think the use case to detect if an object is a FluentSchema makes sense but not sure about the best implementation between Symbol vs Boolean.

We should only have one approach and if Symbol works I would keep it rather than changing the API.

@delvedor, you have originally proposed the Symbol approach do you have any suggestion?

jsumners · 2019-08-03T14:58:47Z

Symbols are useful when the property doesn’t need to be exposed during serialization or to keep the property “private”. Neither situation applies here. The regular property is less mysterious and easier to use.

mcollina · 2019-08-03T15:19:13Z

+1 on using a property Il giorno sab 3 ago 2019 alle 16:58 James Sumners <notifications@github.com> ha scritto:

…

Symbols are useful when the property doesn’t need to be exposed during serialization or to keep the property “private”. Neither situation applies here. The regular property is less mysterious and easier to use. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMXY244S3QB4YMAY2Q27DQCWMKPANCNFSM4IIGYR5Q> .

jsumners · 2019-08-07T16:21:55Z

@delvedor are you +1 for using a plain property?

aboutlo · 2019-08-08T10:28:32Z

OK, given there is a general consensus if no news hopefully this evening I will move forward with the boolean approach

Add boolean to detect object type

6c0cf9a

mcollina requested review from aboutlo and delvedor August 1, 2019 07:32

mcollina approved these changes Aug 1, 2019

View reviewed changes

delvedor approved these changes Aug 1, 2019

View reviewed changes

jsumners added 2 commits August 3, 2019 07:54

Add integration test

96138dd

Adjust readme according to feedback

44a5b49

jsumners force-pushed the non-symbol branch from 618eeec to 44a5b49 Compare August 3, 2019 11:56

Update utils checker

aa172e4

Fix null schema

39f4f7d

aboutlo approved these changes Aug 8, 2019

View reviewed changes

aboutlo merged commit dfd65d8 into fastify:master Aug 8, 2019

jsumners deleted the non-symbol branch August 8, 2019 23:41

jsumners mentioned this pull request Aug 10, 2019

Use .isFluentSchema instead of symbol to check for fluent-schema fastify/fastify#1794

Merged

4 tasks

Uh oh!

Add boolean to detect object type #40

Add boolean to detect object type #40

Uh oh!

Conversation

jsumners commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jul 31, 2019

Uh oh!

jsumners commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jul 31, 2019 via email

Uh oh!

jsumners commented Jul 31, 2019

Uh oh!

jsumners commented Jul 31, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 1, 2019 via email

Uh oh!

jsumners commented Aug 1, 2019

Uh oh!

mcollina commented Aug 1, 2019

Uh oh!

aboutlo commented Aug 1, 2019 via email

Uh oh!

aboutlo commented Aug 3, 2019

Uh oh!

jsumners commented Aug 3, 2019

Uh oh!

aboutlo commented Aug 3, 2019

Uh oh!

jsumners commented Aug 3, 2019

Uh oh!

mcollina commented Aug 3, 2019 via email

Uh oh!

jsumners commented Aug 7, 2019

Uh oh!

aboutlo commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsumners commented Jul 31, 2019 •

edited

Loading

jsumners commented Jul 31, 2019 •

edited

Loading