Skip to content

Conversation

delvedor
Copy link
Member

Using example.js as schema, this is the generated code:

function $main(obj) {
      var json = '{'

      if (obj.firstName !== undefined) {
        json += '"firstName":'
        json += $asString(obj.firstName)
        json += ','
      }

      if (obj.lastName !== undefined) {
        json += '"lastName":'
        json += $asString(obj.lastName)
        json += ','
      }

      if (obj.age !== undefined) {
        json += '"age":'
        json += $asNumber(obj.age)
        json += ','
      }

      if (obj.now !== undefined) {
        json += '"now":'
        json += $asString(obj.now)
        json += ','
      } else {
        throw new Error('now is required!')
      }

      if (obj.reg !== undefined) {
        json += '"reg":'
        json += $asString(obj.reg)
      }

      json += '}'
      return json
    }

If a parameter is required, there will be the else branch.

As you can see I'm not using obj.hasOwnProperty(prop) for testing the existence of the key (is commented inside the code), because I've noticed a big difference of performances.
When using obj.key !== undefined I get around 5 million ops/sec, instead if I use obj.hasOwnProperty(prop) I get ~2 million ops/sec.

The problem is that obj.key !== undefined is not strict as obj.hasOwnProperty(prop).
Opinions?

index.js Outdated
json += '${$asString(key)}:'
`
if (obj.${key} !== undefined) {
json += '${$asString(key)}:'`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please close the '`' in the new line

@mcollina
Copy link
Member

I agree on using !== undefined, if you are using a schema, you want that type. On the other end, I see why people would want that behavior. Put a comment to this PR in that place, something like "using !== for perf reasons, see #3 for discussion".

I'm 100% open to have a configuration switch to use hasOwnProperty but I don't think it's important at this stage of the project. So, if someone wants to implement it and send a PR, I'm happy to review it and merge.

Anyway, this is a major difference from JSON.stringify, so we should write it in the README.md.

@delvedor
Copy link
Member Author

Totally agree with you, in the next commit I'll add the README and code changes.

@mcollina mcollina merged commit 8287e68 into master Aug 22, 2016
@mcollina mcollina deleted the missing-values branch August 22, 2016 13:49
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.

2 participants