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

Javascript: migrate prototypical ATN objects to ES6 #2771

Merged
merged 22 commits into from Mar 9, 2020

Conversation

carocad
Copy link
Contributor

@carocad carocad commented Mar 8, 2020

This PR is a follow up on #2749.

The scope of this PR is to refactor the ATN object declarations to:

  • use es6 classes/inheritance instead of direct prototype manipulation. Es6 class declarations are really just syntactic sugar over prototype inheritance so this change should have no effect on the runtime itself.
  • refactor exports to directly expose a single class whenever possible
  • use const/let on variable declarations to improve variable scoping
  • refactor comments to be JsDoc compatible which most IDEs can leverage to provide better developer feedback

NOTE: I also renamed this "constructor" to be the equals method as it made no sense to have two constructors and for consistency with the SemanticContext.java implementation

fix: put the documentation on the method, not on the import
fix: move Token import to the top
use module.exports
use const/let
use jsdoc
fix: dont wrap class in an object
refactored imports accordingly
use module.exports
use jsdoc
fix: dont wrap the class in an object for export
use const for better scoping
fix: dont wrap class in object for export
use const for better scoping
fix: dont wrap class in object for export
use const/let for better scoping
fix: dont wrap class in object for export
use const/let for better scoping
use object destructuring
fix: dont wrap class in object for export
use const/let for better scoping
use object destructuring
fix: dont wrap class in object for export
use const/let for better scoping
use object destructuring
use jsdoc
use const for better scoping
@ericvergnaud
Copy link
Contributor

Hi,

thanks for this

re "NOTE: I also renamed this "constructor" to be the equals method as it made no sense to have two constructors and for consistency with the SemanticContext.java implementation"
I couldn't find the corresponding change (the link doesn't lead to a specific chunk)

Cheers

@carocad
Copy link
Contributor Author

carocad commented Mar 8, 2020

@ericvergnaud it seems that github doesnt show the file due to the file being "collapsed" i.e diff too big.

Anyway, here is the actual diff.

Specifically this below:

OR.prototype = Object.create(SemanticContext.prototype);
OR.prototype.constructor = OR;

OR.prototype.constructor = function(other) {
	if (this === other) {
		return true;
	} else if (!(other instanceof OR)) {
		return false;
	} else {
		return this.opnds === other.opnds;

As you can see the code declares the constructor of OR as being the function OR but then it overwrites it with another "constructor" which checks if the two objects are equal.

@ericvergnaud
Copy link
Contributor

it does look like a bug, but then I wonder how the whole thing works...

@ericvergnaud
Copy link
Contributor

maybe we're lacking some tests for semantic predicates

@carocad
Copy link
Contributor Author

carocad commented Mar 8, 2020

it does look like a bug, but then I wonder how the whole thing works...

a simple

const foo = new OR("a", "b")
console.log(foo)

at the end of the SemanticContext.js file shows that the constructor isnt called when calling new bur rather the "real" constructor is being called. It simply sets the values on the prototype chain. I guess that this was working due to that "misconception" 🤷‍♂

@ericvergnaud
Copy link
Contributor

@parrt blessed

@parrt parrt merged commit 1284814 into antlr:master Mar 9, 2020
@carocad carocad deleted the atn-es6 branch March 9, 2020 16:18
@ericvergnaud
Copy link
Contributor

@carocad if not already done, would you be able to provide a doc for using this ES6 version locally?
I've tried npm link, which ends up badly (ES2015 functions inheriting from ES6 classes...)
I've tried webpack, but that creates a browser only minimized file

@carocad
Copy link
Contributor Author

carocad commented Jul 13, 2020

would you be able to provide a doc for using this ES6 version locally?

Not sure what you mean. Locally it is simply using node.js so a standard const antlr4 = require('./src/antlr4') would do (assuming that you are in the js runtime dir).

That is also what the test suite does. Generates the code and copies the js runtime locally so that it is available.

I've tried npm link, which ends up badly (ES2015 functions inheriting from ES6 classes...)

mmmm 🤔 . I tried npm link as per the official documentation and it worked as expected. I was able to import the library with const antlr4 = require('antlr4'). Nodejs displays the antlr4 object as containing functions however I think that is normal; as classes are just sugared functions.

I could do an extra guide on how to use it but right now I don't know exactly what to put there as it is simply "require it as you would any other node.js library" 🤷‍♂️

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jul 14, 2020 via email

@parrt parrt added this to the 4.9 milestone Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants