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

feat: add a traverse schema function #198

Merged
merged 14 commits into from
Dec 10, 2020

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Nov 17, 2020

Description

The PR contains the following updates

  • Exposed a traverseSchema function
  • Split out the mark recursive functionality
  • Added test to the traverseSchema function
  • Added special iterator type to include certain schemas
  • Revisited all the code which recursively tries to access schemas

Related issue(s)
Fixes #166

@derberg
Copy link
Member

derberg commented Nov 17, 2020

@jonaslagoni awesome you picked it up. Let me know once you are ready for some hands on, I'd like to test it on the template for templates, and basically remove https://github.com/asyncapi/template-for-generator-templates/blob/main/filters/mermaidDiagram.js#L75-L177

@jonaslagoni
Copy link
Member Author

Hmm, this is not as straight forward as I first assumed since it is implemented with markcircular on the previous schema not the currently crawled schema. And its not possible to directly overwrite the schema because of the nature of JavaScript passes objects as copy of a reference, have to do some sort of recursive overwrite in that case 🤔

@derberg
Copy link
Member

derberg commented Nov 18, 2020

@jonaslagoni oh yes, I think some refactor might be needed, not trivial one, this is why I've put it here and didn't work on it immediately

@jonaslagoni jonaslagoni marked this pull request as ready for review November 20, 2020 15:08
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Awesome! I only have a suggestion to change cb name to more verbose callback :)

lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Outdated Show resolved Hide resolved
lib/models/asyncapi.js Show resolved Hide resolved
magicmatatjahu
magicmatatjahu previously approved these changes Nov 23, 2020
@jonaslagoni jonaslagoni changed the title Recursive change feat: Added a traverse schema function Nov 23, 2020
@derberg
Copy link
Member

derberg commented Nov 27, 2020

@jonaslagoni I'll give it a try in template for templates. I'll keep ya posted

@derberg
Copy link
Member

derberg commented Nov 27, 2020

so far I'm running it locally without introduction any changes to template, like this:

../generator/cli.js https://raw.githubusercontent.com/asyncapi/generator/v1.0.1/test/docs/dummy.yml ./ -o output
  • where ../generator/cli.js points to my local generator with parser pointing to your fork
  • where ./ indicates I use local template which is template for templates

I get this error:

tl;dr property of type string, inside object, doesn't get anonymousId assigned

Something went wrong:
Template render error: (/Users/wookiee/sources/generator/node_modules/template-for-generator-templates/template/index.html)
  Template render error: (/Users/wookiee/sources/generator/node_modules/template-for-generator-templates/partials/diagramContent.html) [Line 13, Column 48]
  TypeError: Cannot read property 'startsWith' of undefined
    at Object._prettifyError (/Users/wookiee/sources/generator/node_modules/nunjucks/src/lib.js:36:11)
    at /Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:561:19
    at eval (eval at _compile (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:631:18), <anonymous>:49:11)
    at /Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:569:11
    at Template.root [as rootRenderFunc] (eval at _compile (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:631:18), <anonymous>:63:3)
    at Template.render (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:550:10)
    at eval (eval at _compile (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:631:18), <anonymous>:48:10)
    at fn (/Users/wookiee/sources/generator/node_modules/a-sync-waterfall/index.js:26:24)
    at /Users/wookiee/sources/generator/node_modules/a-sync-waterfall/index.js:66:22
    at executeSync (/Users/wookiee/sources/generator/node_modules/a-sync-waterfall/index.js:8:15)
    at /Users/wookiee/sources/generator/node_modules/a-sync-waterfall/index.js:65:11
    at eval (eval at _compile (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:631:18), <anonymous>:44:1)
    at createTemplate (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:313:9)
    at handle (/Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:325:11)
    at /Users/wookiee/sources/generator/node_modules/nunjucks/src/environment.js:337:9
    at next (/Users/wookiee/sources/generator/node_modules/nunjucks/src/lib.js:280:7)

Of cource when I try the same with latest generator, all is good. Something is wrong here https://github.com/asyncapi/template-for-generator-templates/blob/main/filters/mermaidDiagram.js#L13. There is a moment when https://github.com/asyncapi/template-for-generator-templates/blob/main/filters/mermaidDiagram.js#L35 gets undefined in propSchemaId. I narrowed it down that problem is with this prop dummyRecursiveProp2 from dummyRecursiveObject schema, it doesn't get anonymousId assigned at all.

I hope this helps. I did not check where exactly is the problem on your side but this investigation should make it easier to find

@jonaslagoni
Copy link
Member Author

There is a moment when https://github.com/asyncapi/template-for-generator-templates/blob/main/filters/mermaidDiagram.js#L35 gets undefined in propSchemaId. I narrowed it down that problem is with this prop dummyRecursiveProp2 from dummyRecursiveObject schema, it doesn't get anonymousId assigned at all.

Found the issue, creating some tests and then pushing the change.

@jonaslagoni
Copy link
Member Author

@derberg done. Tried testing it with your document and it seems to work. return is a dangerous thing inside a loop that should continue 🙄

@derberg
Copy link
Member

derberg commented Nov 30, 2020

  • I wonder if we could get schemaTypesToIterate also into API docs, like have a list of supported types?
  • I get strange results after your changes, results in diagram. Below you can find diagram before and after your change. Look at dummyObject.
    After:

Screenshot 2020-11-30 at 17 11 45

Before:

Screenshot 2020-11-30 at 18 18 08

dummyObject.dummyObjectProp2 is a reference to dummyRecursiveObject, and we know dummyRecursiveObject has recursive property so dummyObject should also list dummyObjectProp2 as circular prop, right? or my thinking is stupid as then it would mean dummyCreated also should list dummyObject as circular prop . Ok, I think you mixed bug with this PR 😄

ok, after checking with html template, I'm now actually sure you fixed a bug 👏
Before:
Screenshot 2020-11-30 at 18 55 57

After:
Screenshot 2020-11-30 at 18 55 39


Now I will focus on checking if I can actually use this new function to refactor my filter

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

now this is what I call a good feature - when by using it I add just one line and remove 108 -> https://github.com/asyncapi/template-for-generator-templates/pull/5/files 👏

I only shared my concerns about jsdocs, that there is one small error I think, and that those special types you added are not represented in jsdoc

lib/models/asyncapi.js Show resolved Hide resolved
@jonaslagoni jonaslagoni requested a review from derberg December 4, 2020 11:55
@jonaslagoni
Copy link
Member Author

jonaslagoni commented Dec 4, 2020

@derberg sorry for requesting review before fixing the lint errors. It is ready now 😄

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Awesome work man!
Let us not merge yet, I want to first fix the issue with twitter action to not tweet again that it this feature was done by the bot 😄
I'll work on it today

@derberg derberg changed the title feat: Added a traverse schema function feat: add a traverse schema function Dec 7, 2020
@derberg
Copy link
Member

derberg commented Dec 8, 2020

@jonaslagoni I'm almost done with #208, one repo left to get pr testing there with mandatory option. I want to merge it first too because we will be able to test with your feature how the automated bump works 😄

@jonaslagoni
Copy link
Member Author

@jonaslagoni I'm almost done with #208, one repo left to get pr testing there with mandatory option. I want to merge it first too because we will be able to test with your feature how the automated bump works 😄

Sounds good 👍

@derberg derberg merged commit d35d59a into asyncapi:master Dec 10, 2020
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jonaslagoni jonaslagoni deleted the recursive_change branch December 15, 2020 14:40
jonastg added a commit to jonastg/parser-js that referenced this pull request Jan 18, 2021
* master: (24 commits)
  chore: Refactored code location for iterators (asyncapi#225)
  chore(release): 1.3.1 (asyncapi#222)
  fix: apply traits to standalone messages from components section (asyncapi#214)
  test: improve feedback loop from browser tests (asyncapi#216)
  ci: fix the space-issue in bump workflow (asyncapi#218)
  ci: update global workflows (asyncapi#217)
  chore(deps): bump ini from 1.3.5 to 1.3.7 (asyncapi#215)
  ci: rename pr testing job name and test node 14 (asyncapi#213)
  ci: bump workflow to start on push instead of release (asyncapi#212)
  chore(release): 1.3.0 (asyncapi#211)
  feat: add a traverse schema function (asyncapi#198)
  ci: add workflow that bumps parser in other asyncapi repos (asyncapi#208)
  ci: fix release workflow step that is responsible for handling twitter (asyncapi#209)
  ci: update global workflows (asyncapi#206)
  ci: disable any testing on draft PR (asyncapi#204)
  chore(release): 1.2.0 (asyncapi#202)
  feat: extend the components and asyncapi model with has-like functions (asyncapi#192)
  chore(deps-dev): bump semantic-release from 17.0.6 to 17.2.3 (asyncapi#199)
  chore(release): 1.1.1 (asyncapi#197)
  fix: channels with name '/' fail on validation (asyncapi#196)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser API should expose functions for traversing through schemas
4 participants