Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

fix: be compatible with the latest minim #481

Merged
merged 6 commits into from
Mar 1, 2019

Conversation

ajkl2533
Copy link
Contributor

No description provided.

@ajkl2533 ajkl2533 requested review from opichals and char0n February 27, 2019 15:48
@opichals
Copy link
Contributor

@ajkl2533 A lot of the fixtures changed just due to sorting of the @style contents. Could we drop those?

I'd only commit the updated fixtures for the tests that were not passing the tests so that we could verify more easily.

this.element = ref.toValue();
},
});
const createReferenceElement = function createReferenceElement(ref) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should investigate why we see the Uncaught TypeError: Class constructor cannot be invoked without 'new'when trying to use class ReferenceElement extends Element {...}`

// cc @kylef

Copy link
Member

Choose a reason for hiding this comment

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

Why not use the RefElement from minim itself?

You can use element.toRef() to create a reference element from another element.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I was following the format fury.parse returns which is { "element": "Address" }. I have never found the 'ref' element in there. Is that equivalent?

Copy link
Member

@kylef kylef Feb 28, 2019

Choose a reason for hiding this comment

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

Oh I see, there are two types of references. There is "ref" element, which is used under some circumstances and then there is placing the id of the target element as your element name which. I though the ReferenceElement here was the same thing as our RefElement.

What is the code with class ReferenceElement extends Element that isn't working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I do this:

const ReferenceElement = class ReferenceElement extends Element {
      constructor(ref) {
        super(ref);

        this.element = ref.toValue();
      }
};

I'll get:

TypeError: Class constructor Element cannot be invoked without 'new'
    at new ReferenceElement (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/src/Components/Attributes/test/Attributes.js:50:24)
    at Suite.<anonymous> (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/src/Components/Attributes/test/Attributes.js:64:16)
    at Object.create (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/interfaces/common.js:114:19)
    at context.describe.context.context (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/interfaces/bdd.js:44:27)
    at Suite.<anonymous> (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/src/Components/Attributes/test/Attributes.js:40:3)
    at Object.create (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/interfaces/common.js:114:19)
    at context.describe.context.context (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/interfaces/bdd.js:44:27)
    at Object.<anonymous> (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/src/Components/Attributes/test/Attributes.js:9:1)
    at Module._compile (module.js:652:30)
    at loader (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/babel-register/lib/node.js:144:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/babel-register/lib/node.js:154:7)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at /Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/lib/mocha.js:514:10)
    at Object.<anonymous> (/Users/radekpodrazky/Documents/PROJECTS/attributes-kit/node_modules/mocha/bin/_mocha:480:18)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:612:3

Copy link
Member

Choose a reason for hiding this comment

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

TypeError: Class constructor Element cannot be invoked without 'new'

I think this is caused by the transpilation introduced in this project, for some reason "Element" is a function and thus you can't new or super it. Take the following example:

$ node example.js
ReferenceElement {
  _content: StringElement { _content: 'User', _storedElement: 'string' },
  _storedElement: 'User' }

$ cat example.js
const { Element, refract } = require('minim');

class ReferenceElement extends Element {
  constructor(ref) {
    super(ref);
    this.element = ref.toValue();
  }
};

const ref = new ReferenceElement(refract('User'));
console.log(ref);

package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@opichals opichals left a comment

Choose a reason for hiding this comment

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

Please nave a look at the comments I added.

@ajkl2533 ajkl2533 force-pushed the ajkl2533/fix-json-serializer branch from 8ff97e9 to 74688bc Compare February 28, 2019 11:43
@ajkl2533 ajkl2533 requested review from opichals and removed request for char0n March 1, 2019 09:06
@opichals
Copy link
Contributor

opichals commented Mar 1, 2019

There is "ref" element, which is used under some circumstances and then there is placing the id of the target element as your element name which

@kylef Is there a doc describing the 'ref' vs 'element' references and under which conditions are they used?

Copy link
Contributor

@opichals opichals left a comment

Choose a reason for hiding this comment

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

Thanks! I'd go for a merge, we can adjust the ReferenceElement in another PR.

@ajkl2533 ajkl2533 changed the title chore(package): update parsing deps fix: fix JSON06Serializer import Mar 1, 2019
@ajkl2533 ajkl2533 changed the title fix: fix JSON06Serializer import chore(package): update parsing deps Mar 1, 2019
@ajkl2533 ajkl2533 changed the title chore(package): update parsing deps fix: be compatible with the latest minim Mar 1, 2019
@ajkl2533 ajkl2533 merged commit 7d53a4f into master Mar 1, 2019
@kylef
Copy link
Member

kylef commented Mar 3, 2019

@kylef Is there a doc describing the 'ref' vs 'element' references and under which conditions are they used?

I think all our parsers will prefer the { "element": "User" } case unless a reference cannot be expressed as such and then it may be expressed as a ref element. These cases may include remote references (which we do not support). Reference to a part of an element, for example reference to the content of another element and not the entire element.

Drafter will use ref elements to express mixins such (one example at: https://github.com/apiaryio/drafter/blob/e83e2fd48ef647bb525e5a95ddaef52c9306ed6b/test/fixtures/render/array-mixin.apib).

The docs for ref element at http://apielements.org/en/latest/element-definitions.html#ref-element.

/c @tjanc you might have some more input on referencing via ref element vs element of id.

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.

3 participants