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

Diagrams with nodejs fail on instanceof check #268

Closed
letmaik opened this issue Aug 22, 2016 · 9 comments
Closed

Diagrams with nodejs fail on instanceof check #268

letmaik opened this issue Aug 22, 2016 · 9 comments

Comments

@letmaik
Copy link

letmaik commented Aug 22, 2016

I'm using chevrotain on nodejs and wanted to generate a diagram for a parser using nodejs workflows. I stumbled upon a serious problem though, which is in the use of instanceof in chevrotain code and how the chevrotain library is require'd within the diagram code.

When I do var chevrotain = require('chevrotain') then this effectively includes lib/src/api.js from chevrotain because this is what the "main" field of chevrotain's package.json references. All fine. However, when I then do var diag = require('chevrotain/diagrams/src/main') then this internally does a require('../../lib/chevrotain') which effectively loads a different chevrotain library.

Working with two different chevrotain objects breaks badly in cases when chevrotain does an instanceof check like here or here.

As a temporary fix, I copied the /diagrams/src folder into my project and changed the diagrams_builder.js file from

module.exports = factory(require('../vendor/railroad-diagrams'), require('../../lib/chevrotain'))

to

module.exports = factory(require('chevrotain/diagrams/vendor/railroad-diagrams'), require('chevrotain'));

So the important bit is the second require. chevrotain instead of ../../lib/chevrotain. With that it works since it uses a single library only. I am not sure if this solution is the best one and works for everyone, but it works for me.

@bd82
Copy link
Member

bd82 commented Aug 22, 2016

Fixing this seems easy.

I should only change the UMD pattern here

to require("chevrotain")
instead of require('../../lib/chevrotain')

What I do not understand though is why you are doing

var diag = require('chevrotain/diagrams/src/main');

The code that generates the diagrams uses the railroad diagrams 3rd party lib.

This code has dependencies to the DOM and I doubt it will run on node.js.
It is more accurate to say that that diagrams are not being generated (as new files created)
rather that they are being rendered.

Due to these limitations the current diagrams workflow is normally.

  • browserify / webpack your grammar.
  • modify diagrams html template to load the relevant resources depending on your project
    including your browserified grammar.
  • open the html template which renders the diagrams.

Is your workflow different?

@letmaik
Copy link
Author

letmaik commented Aug 22, 2016

I don't run the diagramming code in node.js, but use browserify for that particular diagramming file. But it is the same code base that otherwise runs on node.js. So what I do is I run browserify like that:

$ browserify diagram.js -u wordnet-db -u lapack -o diagram.bundle.js

(have to exclude wordnet-db and lapack as those optional chevrotain dependencies get picked up by browserify and would fail since I don't use them and haven't installed them) see comment below

And then I have the following HTML:

<!DOCTYPE html>
<meta charset="utf-8">
<style>
    body {
        background-color: hsl(30, 20%, 95%)
    }
</style>

<link rel='stylesheet' href='../node_modules/chevrotain/diagrams/diagrams.css'>

<body>
    <div id="diagrams" align="center"></div>

    <script src='diagram.bundle.js'></script>
</body>

The diagram.js file looks like that:

var main = require('./diagrams/main'); // my local fixed copy
var MyParser = require('./parser');
var parserInstanceToDraw = new MyParser([]);
var diagramsDiv = document.getElementById("diagrams");
main.drawDiagramsFromParserInstance(parserInstanceToDraw, diagramsDiv);

@bd82
Copy link
Member

bd82 commented Aug 22, 2016

o.k.

So you are building your template in a different way, no worries I see no reason not to support this as well. I will try to modify the diagrams's code UMD pattern to use require("chevrotain")
so it would resolve to the same node module when browserifying and thus will avoid
having two separate prototype chains when running in node.js which causes the instanceof operator to fail.

(have to exclude wordnet-db and lapack as those optional chevrotain dependencies get picked up by browserify and would fail since I don't use them and haven't installed them)

Chevrotain has optional dependencies ? This is new to me... 😄 did you mean something else?

@letmaik
Copy link
Author

letmaik commented Aug 22, 2016

Thanks! Sounds good.

(have to exclude wordnet-db and lapack as those optional chevrotain dependencies get picked up by browserify and would fail since I don't use them and haven't installed them)

Chevrotain has optional dependencies ? This is new to me... 😄 did you mean something else?

Sorry, mixed something up there. These are actually optional dependencies of the natural library. My fault.

@bd82 bd82 closed this as completed in 5c10ff8 Aug 22, 2016
@bd82 bd82 reopened this Aug 22, 2016
@bd82
Copy link
Member

bd82 commented Aug 22, 2016

Allright @letmaik.

Can you please check if commit 5c10ff8 fixes your issue?
(just perform the same modification in node_modules/chevrotain/diagrams)

Seems to work locally for me on node.js version 6, but I am just requiring it, I do not perform packaging.
Do you have another version of node.js?
I will test this tomorrow on node.js version 4 as well before releasing.

I am wondering if your method of packaging and creating the diagrams is easier to use versus the one documented in Chevrotain. or if possibly there is no "best" way and each user should be left to his/hers own devices?

@letmaik
Copy link
Author

letmaik commented Aug 23, 2016

Yes that works for me.

I think my method is easier the moment you have a few more dependencies in your parser module, like using additional libraries or depending on other modules in your project. So if you use node and npm anyway, then I think this is the proper way. If you don't, it may be too much overhead to set it up.

@bd82
Copy link
Member

bd82 commented Aug 23, 2016

@letmaik version 0.13.2 is out on npm which includes the bug fix.

Regarding easier packaging for the diagrams, I think this would be resolved once I find the time to implement serialization and de-serialization of the grammar, thus allowing rendering the diagrams without browser packaging.

Instead simply using a script to generate the serialized form and load it in the browser for rendering purposes. So no need to package your entire parser including its dependencies for use in the browser.

@bd82 bd82 closed this as completed Aug 23, 2016
@letmaik
Copy link
Author

letmaik commented Aug 24, 2016

Thanks for the quick release!

I agree about your plan with generating diagrams directly. This would make things a lot simpler, especially when autogenerating bits of documentation.

@bd82
Copy link
Member

bd82 commented Aug 24, 2016

Thanks for the quick release!

Releasing is easy when you are lazy and automate everything 😄

And thanks for the feedback.

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

No branches or pull requests

2 participants