Amphtml: amp-bind: Allow trailing commas in literals

[choumx]

First Timers Only

We know figuring out the process for contributing to an open source project can be intimidating, so we created this issue as a way for you to learn the ropes. (If you feel comfortable contributing to open source projects, please leave this issue for someone else.)

What you will need to know

• Some familiarity with JS.

Background

amp-bind is a new AMP extension that adds custom interactivity via data-binding and expressions. The expression language is a subset of JS and includes array and object literals.

Motivation

Currently, trailing commas are valid ES5 JS but are not valid in amp-bind expressions. This is standard style convention that should be supported and will enable cleaner code.

[1, 2, 3,] // Trailing comma after "3".

{a: 'a', b: 'b', c: 'c',} // Trailing comma after "c".

The bug

Update amp-bind's expression language to support trailing commas! 😃

Step by step

• [x] Claim this issue by adding a comment below. Please only claim this bug if you plan on starting work in the next day or so.
• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] Update bind-expr-impl.jison to support trailing commas in array and object literals. Hint: Add a new clause to the array and object rules.
• [ ] Run gulp compile-bind-expr to recompile the Jison grammar into JS (bind-expr-impl.js).
• [ ] Add corresponding unit tests to test-bind-expression.js.
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Mention Fixes #10404. in the description and mention @choumx in the PR description.
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

Once approved, your changes will be merged. ⚡⚡⚡Congrats on making your first contribution to the AMP Project!⚡⚡⚡ You'll be able to see it live across the web soon!

Thanks, and we hope to see more contributions from you soon.

Questions?

If you have questions ask in this issue or on your Pull Request (if you've created one) or see the How to get help section of the Getting Started guide. You can also add a comment to this issue and mention me @choumx with your question.

[thechinedu]

hi @choumx, I'd very much like to work on this issue. Can I proceed?

[choumx]

@blueyedgeek Great! Go for it. 👍

[thechinedu]

Hi @choumx,

I'm having issues getting the tests to run locally (I haven't made any changes yet), I keep getting the error: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

What can I do about this?

[choumx]

@blueyedgeek Which tests are timing out? Can you show a Gist with the command and full output? Thanks.

[thechinedu]

Sure thing. Here's a gist: https://gist.github.com/blueyedgeek/5e29b34f9b26d36f7311ab68783e91c7

[choumx]

Thanks. We've had some issues with local test flakiness and your errors looks related. Please ignore those for now while we investigate.

[thechinedu]

I won't be able to test if my additions to the parser generator actually work if I am unable to run tests. Is there some other way for me to manually test that if my additions work or not?

[choumx]

Try syncing now that #10473 is merged. You can safely ignore tests unrelated to amp-bind, e.g. run:

gulp test --files=extensions/amp-bind/0.1/test/test* --watch

Also, once you create a PR, the full test suite will be run on Travis which may be more predictable for timeout-risk tests (depending on your personal machine's configuration).

Sorry about the hassle and thanks for contributing! 🙏