search

EricSmekens / jsep

JavaScript Expression Parser
http://ericsmekens.github.io/jsep/
MIT License
827 stars 133 forks source link

Basic Arrow function support + array methods #123

Closed 6utt3rfly closed 3 years ago

6utt3rfly commented 3 years ago

NOTE: Objects and properties aren't supported, so () => ({ a: 1 }) will fail

Examples:

  1. multi-param arrow function: a.find((val, key) => key === "abc")

    {
"type": "CallExpression",
"arguments": [
{
  "type": "ArrowFunctionExpression",
  "params": [
    {
      "type": "Identifier",
      "name": "val"
    },
    {
      "type": "Identifier",
      "name": "key"
    }
  ],
  "body": {
    "type": "BinaryExpression",
    "operator": "===",
    "left": {
      "type": "Identifier",
      "name": "key"
    },
    "right": {
      "type": "Literal",
      "value": "abc",
      "raw": "\"abc\""
    }
  }
}
],
"callee": {
"type": "MemberExpression",
"computed": false,
"object": {
  "type": "Identifier",
  "name": "a"
},
"property": {
  "type": "Identifier",
  "name": "find"
}
}
}

  2. no parentheses around single arrow function argument: a.find(val => key === "abc")

{
  "type": "CallExpression",
  "arguments": [
    {
      "type": "ArrowFunctionExpression",
      "params": [
        {
          "type": "Identifier",
          "name": "val"
        }
      ],
      "body": {
        "type": "BinaryExpression",
        "operator": "===",
        "left": {
          "type": "Identifier",
          "name": "key"
        },
        "right": {
          "type": "Literal",
          "value": "abc",
          "raw": "\"abc\""
        }
      }
    }
  ],
  "callee": {
    "type": "MemberExpression",
    "computed": false,
    "object": {
      "type": "Identifier",
      "name": "a"
    },
    "property": {
      "type": "Identifier",
      "name": "find"
    }
  }
}

  1. no arg arrow function: a.find(() => true)

    {
"type": "CallExpression",
"arguments": [
{
  "type": "ArrowFunctionExpression",
  "params": null,
  "body": {
    "type": "Literal",
    "value": true,
    "raw": "true"
  }
}
],
"callee": {
"type": "MemberExpression",
"computed": false,
"object": {
  "type": "Identifier",
  "name": "a"
},
"property": {
  "type": "Identifier",
  "name": "find"
}
}
}

  2. array function: [1, 2].find(v => v === 2)

    {
"type": "CallExpression",
"arguments": [
{
  "type": "ArrowFunctionExpression",
  "params": [
    {
      "type": "Identifier",
      "name": "v"
    }
  ],
  "body": {
    "type": "BinaryExpression",
    "operator": "===",
    "left": {
      "type": "Identifier",
      "name": "v"
    },
    "right": {
      "type": "Literal",
      "value": 2,
      "raw": "2"
    }
  }
}
],
"callee": {
"type": "MemberExpression",
"computed": false,
"object": {
  "type": "ArrayExpression",
  "elements": [
    {
      "type": "Literal",
      "value": 1,
      "raw": "1"
    },
    {
      "type": "Literal",
      "value": 2,
      "raw": "2"
    }
  ]
},
"property": {
  "type": "Identifier",
  "name": "find"
}
}
}
LeaVerou commented 3 years ago

Hey @6utt3rfly, thank you so much for your efforts to extend jsep!

@EricSmekens feel free to correct me, but I believe these are out of scope? However, we should eventually make jsep extensible, because "expression syntax" means different things to different use cases. Then we could add optional arrow function support.

6utt3rfly commented 3 years ago

Hey @LeaVerou ! Thank you for the speedy response! I wasn't sure if it was out of scope based on #49 ? I initially tried to treat the arrow function with addBinaryOp, but I came across several parsing errors, which lead me to this PR.

'(a, b) => 1' // Unclosed ( at character 11. ...but '() => 1' and 'v => v' work
'[1, 2].length === 2' // Variable names cannot start with a number (.l) at character 7

Making jsep extensible would be interesting. I reviewed other forks and some have added some object support, or back-ticks (I haven't seem template supported yet(`hi ${name}`), which would also be nice. Beyond that, I agree with keeping more complex items out (functions, classes, new, assignment, async/await, etc), although adding an ability to somehow support keywords like new, async, await by extension would be nice, too.

EricSmekens commented 3 years ago

We decided to keep it more simple, as the project is being maintained, but there is not a clear plan on what features we would like to introduce, or determine as out of scope for this library. (There are alternatives which solves more complex scenario's).

For me, this falls right into between those two. I like the pull-request you've made, and I really agree with Lea about extensibility. But at this moment I don't have the energy to dive into making jsep extensible for those kind of features yet. Clear ideas are always welcome ofcourse!

Lea, how do you feel about accepting this PR and creating a new version soon? (I have the time to release a new version etc. It's just the question if we want to go that way). I've the feeling this doesn't break any other correct parsing at this moment, and I think it is anice addition for now, until we make this more extensible.

amb26 commented 3 years ago

I'm interested in this functionality and would welcome seeing it merged. I'm not in any huge hurry, but just thought I would add my vote :)

EricSmekens commented 3 years ago

Another review on this, I would be happy to merge this functionality in jsep, if it would be in a extensible way. I"m also happy to merge this in if lots of people will value this worthy, and we can just add a backwards-compatibilty warning that some behaviour is different now.

6utt3rfly commented 3 years ago

@EricSmekens - I can resolve conflict with the latest updates. I've also been thinking of how to do this in an extensible way... For arrow support, there are 2 main changes needed: 1) Allowing recognition of => to give a new ArrowFunctionExpression 2) Allowing multiple expressions in an argument list without an identifier (the changes to gobbleGroup)

1 - could be made into an extensible param to jsep. I'm thinking of some way that could support both ternary and arrow functions (even if the current ternary code currently in there was left alone, it would prove the design of the extensible system. Moving into the system could allow it to be removed though). For this system, I'm thinking maybe it could just allow any arbitrary function pointer bound to this...? Seems better than trying to define an expression syntax by parameters and lets the caller use the built-in methods. What do you think?

The code blocks I'm talking about are in gobbleExpression, and show why a function pointer might be best?:

if(ch === QUMARK_CODE) {
  // Ternary expression: test ? consequent : alternate
  index++;
  consequent = gobbleExpression();
  if(!consequent) {
    throwError('Expected expression', index);
  }
  gobbleSpaces();
  if(exprICode(index) === COLON_CODE) {
    index++;
    alternate = gobbleExpression();
    if(!alternate) {
      throwError('Expected expression', index);
    }
    return {
      type: CONDITIONAL_EXP,
      test: test,
      consequent: consequent,
      alternate: alternate
    };
  } else {
    throwError('Expected :', index);
  }
} else if(ch === EQUAL_CODE) {
  // arrow expression: () => expr
  index++;
  if(exprICode(index) === GTHAN_CODE) {
    index++;
    return {
      type: ARROW_EXP,
      params: test ? [test] : null,
      body: gobbleExpression(),
    };
  }
} else {
  return test;
}

2 - feels difficult to make extensible except maybe by a boolean enable flag?

Let me know your thoughts and I can update the PR :-)

6utt3rfly commented 3 years ago

I added optional expression_parsers, if something along these lines works?

EricSmekens commented 3 years ago

Left 1 comment, looks quite promising.

I'll leave it open for a few more days, maybe @LeaVerou has an opinion about this as well.

LeaVerou commented 3 years ago

This is a good implementation, but I still think arrow functions should be an optional add-on, and are not desirable in many, if not most, of expression use cases. We don't even support object literals, and we're going to support arrow functions by default?

@6utt3rfly If I prioritize the work to make jsep extensible and get it done this coming week, would you be ok to port your changes to an official plugin? Could we separate the changes to add arrow functions from the changes to move towards an expression_parsers data structure?

6utt3rfly commented 3 years ago

@LeaVerou - I took a look at PrismJS (mentioned in #137 ) and took a stab at adding some hooks. I was able to move the arrow-function support into a plugin, and also added plugins for object support, assignment, and new, (while also moving the ternary code into a default plugin), just to see if the hooks were sufficient. I'm happy to update however it's needed, and welcome any feedback! :-)

LeaVerou commented 3 years ago

Oh wow, lots of good work here! Thank you!!

I have a paper deadline tomorrow, but can review later in the week.

Btw, Prism's hooks implementation is a little old at this point, this is closer to the code I've been using for more recent projects: https://github.com/LeaVerou/bliss/blob/v2/src/Hooks.js Same API with some minor conveniences.

Also if it's not too much trouble, please follow the existing coding style. E.g. we use tabs for indentation (and spaces for alignment). I can fix issues for you after merging (I wouldn't want to hold up such a good PR due to such minor issues), but I figured you may prefer to fix them yourself before merging. ESLint should help, we have an ESLint config now, so if you have an ESLint plugin in your editor it should pick it up automatically.

6utt3rfly commented 3 years ago

@LeaVerou - I updated the tab/spacing and added the plugins folder to the lint script. Sorry about that! I switched the plugin API as requested, too. I also added an ignoreComments plugin (#121 ), and templateLiteral.

I didn't do anything to add the plugins to the build system, so that would still be needed. Or feel free to let me know if anything else needs to be done :-)

6utt3rfly commented 3 years ago

I'm also missing tests for these plugins!

I added tests to the main tests.js file, but it felt wrong and I'd gladly move them! Would you like tests in the same folder as the plugin? Or sub-folders of the tests folder? I use jest in most of my other projects, but would you be interested in adding istanbul and nyc for code coverage?

LeaVerou commented 3 years ago

I added tests to the main tests.js file, but it felt wrong and I'd gladly move them! Would you like tests in the same folder as the plugin? Or sub-folders of the tests folder? I use jest in most of my other projects, but would you be interested in adding istanbul and nyc for code coverage?

Ideally each plugin should have its own separate tests, but I'm not familiar enough with QUnit to know if that's easily feasible. I haven't used jest, istanbul, or nyc before, but from a quick look they look cool. If you're willing to spend the time to switch from QUnit to them, I'm sure me and @EricSmekens would be happy to merge that PR (Eric correct me if not!)!

6utt3rfly commented 3 years ago

If you're willing to spend the time to switch from QUnit to them, I'm sure me and @EricSmekens would be happy to merge that PR (Eric correct me if not!)!

... maybe later. There are enough other things to do first ;-)

6utt3rfly commented 3 years ago

Closing for new plugin architecture and moving into individual PRs:

147

148

149

150

151