ldthomas/apg-js

`function exports(` is controversial.

naugtur opened this issue ยท 7 comments

Hi! ๐Ÿ‘‹

Firstly, thanks for your work on this project! ๐Ÿ™‚

Today I used patch-package to patch apg-js@4.1.1 for the project I'm working on.

I've been trying to build the project using parcel and one of the parsers involved in that was exploding because exports has a meaning and naming something else that was tripping it up. Giving a function a name means it's available under that name in the scope even if it was declared inline, so some parsers will have trouble with it as redefining exports. Meanwhile, the name is not useful for anything as far as I can see.

Here is the diff that solved my problem:

diff --git a/node_modules/apg-js/src/apg-api/scanner.js b/node_modules/apg-js/src/apg-api/scanner.js
index bde1a2b..63ab73e 100644
--- a/node_modules/apg-js/src/apg-api/scanner.js
+++ b/node_modules/apg-js/src/apg-api/scanner.js
@@ -13,7 +13,7 @@
 // - catalog the lines - create an array with a line object for each line.
 // The object carries information about the line number and character length which is used
 // by the parser generator primarily for error reporting.
-module.exports = function exports(chars, errors, strict, trace) {
+module.exports = function (chars, errors, strict, trace) {
   const thisFileName = 'scanner.js: ';
   const apglib = require('../apg-lib/node-exports');
   const grammar = new (require('./scanner-grammar'))();
diff --git a/node_modules/apg-js/src/apg-api/semantic-callbacks.js b/node_modules/apg-js/src/apg-api/semantic-callbacks.js
index d57125b..7407ed1 100644
--- a/node_modules/apg-js/src/apg-api/semantic-callbacks.js
+++ b/node_modules/apg-js/src/apg-api/semantic-callbacks.js
@@ -7,7 +7,7 @@
 // See:<br>
 // `./dist/abnf-for-sabnf-grammar.bnf`<br>
 // for the grammar file these callback functions are based on.
-module.exports = function exports() {
+module.exports = function () {
   const apglib = require('../apg-lib/node-exports');
   const id = apglib.ids;
 
diff --git a/node_modules/apg-js/src/apg-api/show-rules.js b/node_modules/apg-js/src/apg-api/show-rules.js
index 7121cb3..281d08c 100644
--- a/node_modules/apg-js/src/apg-api/show-rules.js
+++ b/node_modules/apg-js/src/apg-api/show-rules.js
@@ -2,7 +2,7 @@
  *   copyright: Copyright (c) 2021 Lowell D. Thomas, all rights reserved
  *     license: BSD-2-Clause (https://opensource.org/licenses/BSD-2-Clause)
  *   ********************************************************************************* */
-module.exports = (function exports() {
+module.exports = (function () {
   const thisFileName = 'show-rules.js';
   // Display the rules.
   // This function may be called before the attributes calculation.
diff --git a/node_modules/apg-js/src/apg-api/syntax-callbacks.js b/node_modules/apg-js/src/apg-api/syntax-callbacks.js
index 992ecce..c007a4b 100644
--- a/node_modules/apg-js/src/apg-api/syntax-callbacks.js
+++ b/node_modules/apg-js/src/apg-api/syntax-callbacks.js
@@ -7,7 +7,7 @@
 // See:<br>
 // `./dist/abnf-for-sabnf-grammar.bnf`<br>
 // for the grammar file these callback functions are based on.
-module.exports = function exports() {
+module.exports = function () {
   const thisFileName = 'syntax-callbacks.js: ';
   const apglib = require('../apg-lib/node-exports');
   const id = apglib.ids;

This issue body was partially generated by patch-package.

Thanks for your interest in apg-js. I'm using ESLint and it complains if a function doesn't have a name. I forget the reason it gives for that but "exports" was probably not the smartest name to use. If I were to update it I would change the name to something innocuous like "exfn" or such but not leave it unnamed. I see it is getting some usage on npm and haven't had any other complaints so for now I'm going to leave you with your private fix. I've been away from this project (and all other coding projects) for a long time now and I'm pretty rusty on using Visual Studio Code, GitHub and npm so I'd need some serious motivation to tackle the learning curve to get back up to speed on all of that again. For now it looks like you have solved your problem. But thanks for letting me know. Maybe I will come back to this problem at a later date.

I did solve it for my use case but it's a somewhat common one. And people who stumble upon it rightfully blame the bundler for the problem and not report anything to you because your code is correct and the bundler failing is not.

I can give you a working pull request that passes your eslint and has the versions bumped where nevessary.
All you need to do is merge it in the UI (without squashing), and npm publish

Would that work? (It's ok to say no)

I did think about asking for a pull request. Yes that would work. But it may take me a few days (or more) to finish the job for you. Thanks for your help.

No rush. I've got some OSS of my own that's been waiting for months to get released

Hi. I felt a little ambitious today and worked my way through changing the function names and publishing version 4.1.2 to GitHub and npm. Give it a try and let me know if it solves your problem. Thanks.

If you have any more problems just open an new issue. I'm closing this for now.

Sorry I didn't let you know earlier, not much hobby time this week.
It works fine. Very cool of you to push it out!