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

Fix .indexOf() call on integers when using numbers as node id's #23

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Fix .indexOf() call on integers when using numbers as node id's #23

merged 1 commit into from
Jul 5, 2022

Conversation

finn-vgtl
Copy link
Contributor

First of all, I'd like to say thank you for this amazing library! Its scope is simply perfect for my use-case.

While implementing redot in my project I've used the following dot notation for testing purposes. (Notice how I used integers for my node id's)

digraph {
  1 [label = A]
  2 [label = B]
  3 [label = C]
  1 -> 2
  2 -> 3
}

Converting this digraph to an AST using redot.parse() worked absolutely perfectly, however, using redot.stringify() to convert the newly generated AST back to a DOT-notation threw an error:

[…]/node_modules/redot-stringify/redot-stringify.js:5
  if (text.indexOf(" ") > 0) {
           ^

TypeError: text.indexOf is not a function
    at utilQuotesAndEscaping ([…]/node_modules/redot-stringify/redot-stringify.js:5:12)
    at nodeId ([…]/node_modules/redot-stringify/redot-stringify.js:58:10)
    at Array.map (<anonymous>)
    at utilProcessChildType ([…]/node_modules/redot-stringify/redot-stringify.js:24:6)
    at nodeStatement ([…]/node_modules/redot-stringify/redot-stringify.js:41:5)
    at Array.map (<anonymous>)
    at utilProcessChildType ([…]/node_modules/redot-stringify/redot-stringify.js:24:6)
    at graph ([…]/node_modules/redot-stringify/redot-stringify.js:94:5)
    at digraph ([…]/node_modules/redot-stringify/redot-stringify.js:114:10)
    at Array.map (<anonymous>)

Node.js v18.2.0

After a bit of debugging, I found out that using strings as node id's won't trigger the error so I've implemented an extra step in the .utilQuotesAndEscaping() function to ensure compatibility with integers in my local setup. Obviously, this is a temporary solution so I'm proposing these changes directly to you:

TL:DR

This PR proposes a small change to convert the text parameter of the function .utilQuotesAndEscaping() to string before trying to call .indexOf() on it, ensuring the latter function is available in the prototype, thus preventing errors.


Thanks for considering my PR! 🙋‍♂️

@finn-vgtl finn-vgtl changed the title Error while calling .indexOf() on integers Fix .indexOf() call on integers when using numbers as node id's Jun 21, 2022
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @finn-vgtl!

@ChristianMurphy ChristianMurphy merged commit c09e4b2 into redotjs:main Jul 5, 2022
@finn-vgtl finn-vgtl deleted the patch-1 branch July 6, 2022 06:19
@Drincann
Copy link

When will this patch be released to the npm repository?

@ChristianMurphy
Copy link
Member

It will be released when the next major is ready, that is pending finalization in #25
Pull requests to help move the next major forward are welcome.
Otherwise be patient.

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

Successfully merging this pull request may close these issues.

3 participants