Skip to content
This repository has been archived by the owner on Dec 2, 2018. It is now read-only.

fix 2 bugs: "type: null" means any type and only Array and Object have to call their default function to get default value. #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iliyaZelenko
Copy link

Also i fix issue for default type.

// Object with a default value
propE: {
  type: Object,
  // Object or array defaults must be returned from
  // a factory function
  default: function () {
    return { message: 'hello' }
  }
},

But if a prop has a type of function, the documentation still calls the function, so the function will not appear in the documentation and even this may cause an error.

This code now works correctly:

Result:

But if the function is large, it does not look comfortable. In this situation, I would do it: I simply showed the function of a button through which all code in a modal window opens to it.

I made a lot of changes so there is a possibility that something will break. But I checked the code in different ways.

The following code worked without errors:

    someProp: {
      type: null,
      default: () => {
        return 123
      },
      note: "test for any type"
    },
    someProp2: {
      type: Function,
      default: () => {
        return mixins.reduce((map, mixin) => {
          Object.assign(map, mixin.props)
          return map
        }, {})
      }
    },
    someProp3: {
      type: Object,
      default: () => {
        return {
          a: 1
        }
      },
      note: "my note"
    },
    someProp4: {
      type: String,
      default: 'My string',
      note: "my note"
    },
    someProp5: {
      type: Array,
      default: () => {
        return {
          b: 2
        }
      }
    },
    someProp6: {
      type: Number,
      default: 32,
      note: "my note"
    },

and gives result:

As you can see, I added displaying as code for "default" column (only for 'array', 'object', 'function') .

@pearofducks
Copy link
Contributor

pearofducks commented Oct 23, 2018

Thanks for adding these fixes!

There are tests associated with the current implementation, do those still pass?

It'd be ideal to see the new functionality included in the tests.

(Looking into why Travis isn't automatically running the tests btw)

@iliyaZelenko
Copy link
Author

Looks like your tests don't work. I cloned your repository, launched yarn and yarn test and got it:

image

I didn't even touch the code, just cloned it.

@pearofducks
Copy link
Contributor

They run fine here. I'm not sure if yarn respects package-lock.json at all, but that could be the source of your problem if you brought in a conflicting dependency.

Travis is able to run master:HEAD just fine - https://travis-ci.org/propellant/doctor

component.vm.getDefault(tComplex.props.second.default).should.be.exactly(JSON.stringify(tComplex.props.second.default()))
component.vm.getDefault(tComplex.props.third.default).should.be.exactly(JSON.stringify(100))
component.vm.getDefault(tComplex.props.fourth.default).should.be.exactly(JSON.stringify('world'))
// default can take different forms, you will need to change this test
Copy link
Author

Choose a reason for hiding this comment

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

default can take different forms, you will need to change this test

@iliyaZelenko
Copy link
Author

iliyaZelenko commented Oct 23, 2018

I commented out one test, it needs to be redone. I rewrote my code differently. The important point: if the default is a function and if there in typeStr (for example "boolean|number|array") is an object or a string in the type list or all of it and still have a function, the function code will be displayed. I also corrected the styles so that there were no stripes in the code, but it would not be bad to display the code as a block.

@pearofducks
Copy link
Contributor

This changeset is pretty significant to just fix a bug. Changing the API that others are relying on means the new behavior would need to make sense and be well-documented. This also means current users might not like the new behavior if they relied on the old.

Right now, as I mentioned, I don't really have time to ensure an API change and major version goes well. So I can't merge this as-is.

I appreciate you putting time into improving this, and if you have time to dedicate to this problem-space I think there are a lot of improvements that could be made on this. Feel free to fork and make amazing things!

@iliyaZelenko
Copy link
Author

iliyaZelenko commented Oct 23, 2018

I didn’t seem to change your API, you have the same methods, I just changed the display of the value from default, it displays through the code tag, before that you would see json on the heap as plain text.

I also fixed 2 bugs:

  • type zero now does not give an error
  • if the default argument is a function, then it will be called only if the type is an array or an object, if the type is a function, then it will not be called, but its code will be output. Also, if there are many types, then it determines how it should be.

In fact, you have one more bug which I did not fix so as not to complicate the code.

Here is what I changed in the component (not visible in the commit):

      getDefault(d, type, objInfo ) {
        const typeStr = this.getType(type)
        const dTypeStr = getTypeString(d)

        if (typeof(d) === 'undefined') return 'undefined'

        // if default is function
        if (dTypeStr === 'function') {
          // if there are types object or array and not function
          if (['array', 'object'].some(i => typeStr.includes(i)) && !typeStr.includes('function')) {
            // get result from function
            const dResult = d()

            objInfo.defaultTypeStr = getTypeString(dResult)
            return JSON.stringify(dResult, null, 2)
          }

          objInfo.defaultTypeStr = 'function'
          // if not array or object then just get function in text format
          return d.toString()
        }

        objInfo.defaultTypeStr = dTypeStr
        // for all other types
        return JSON.stringify(d)
      },
      // works for all types
      getType(t) {
        // for null and undefined
        if (t == undefined) return 'any'

        if (getTypeString(t) === 'function') {
          return getTypeString(t())
        }
        if (Array.isArray(t)) {
          return t.map(this.getType).join('|')
        }

        return getTypeString(t)
      },

I added this function:

  function getTypeString (variable) {
    return Object.prototype.toString.call(variable).slice(8, -1).toLowerCase()
  }

added to component data:

typesForCodeTag: ['array', 'object', 'function']

and changed this part of html:

        <div class="propcol default">
          <!--optionally you can output this: {{ propinfo.defaultTypeStr }} -->
          <code
            v-if="typesForCodeTag.includes(propinfo.defaultTypeStr)"
            style="white-space: pre-wrap;"
          >{{ propinfo.default }}</code>
          <span v-else>{{ propinfo.default }}</span>
        </div>

and a bit in processProps method, not much.

After that I commented out one test and changed one line in the CSS which does not affect anything.

Compatibility with old versions should be, in html the same is displayed but without bugs and more beautifully.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants